Enforce the use of a single string formatting mechanism for translation source strings

.circleci/config.yml

@@ -22,6 +22,7 @@ jobs:
       - run: pipenv run flake8
       - run: pipenv run isort --check-only --diff .
       - run: pipenv run black --target-version py37 --check --diff .
+      - run: pipenv run semgrep --config .semgrep.yml --error .
       - run: git ls-files '*.html' | xargs pipenv run djhtml --check
       - run: pipenv run curlylint --parse-only wagtail
       - run: pipenv run doc8 docs

.pre-commit-config.yaml

@@ -50,3 +50,8 @@ repos:
     rev: v1.4.13
     hooks:
       - id: djhtml
+  - repo: https://github.com/returntocorp/semgrep
+    rev: v0.117.0
+    hooks:
+      - id: semgrep
+        args: ['--config', '.semgrep.yml', '--error']

.semgrep.yml

@@ -0,0 +1,79 @@
+rules:
+  - id: translation-no-new-style-formatting
+    patterns:
+      - pattern: $FUNC("$STRING_ID", ...)
+      - metavariable-regex:
+          metavariable: $FUNC
+          regex: '_|gettext|gettext_lazy|ngettext|ngettext_lazy'
+      - metavariable-regex:
+          metavariable: $STRING_ID
+          regex: ".*({(\\d*|[\\w_]*)}).*"
+    message: |
+      Do not use str.format style formatting for translations.
+      Use printf style formatting with named placeholders instead.
+      For example, do `_("Hello %(name)s") % {"name": "Wagtail"}`
+      instead of `_("Hello {name}").format(name="Wagtail")`.
+      See https://docs.wagtail.org/en/latest/contributing/translations.html#marking-strings-for-translation for more information.
+    languages: [python, javascript, typescript]
+    severity: ERROR
+  - id: translation-no-f-strings
+    patterns:
+      - pattern: $FUNC(f"...", ...)
+      - metavariable-regex:
+          metavariable: $FUNC
+          regex: '_|gettext|gettext_lazy|ngettext|ngettext_lazy'
+    message: >
+      Do not use formatted string literals for translations.
+      Use printf style formatting with named placeholders instead.
+      For example, do `_("Hello %(name)s") % {"name": "Wagtail"}`
+      instead of `_(f"Hello {name}")`.
+      See https://docs.wagtail.org/en/latest/contributing/translations.html#marking-strings-for-translation for more information.
+    languages: [python]
+    severity: ERROR
+  - id: translation-no-anonymous-arguments
+    patterns:
+      - pattern: $FUNC("$STRING_ID", ...)
+      - metavariable-regex:
+          metavariable: $FUNC
+          regex: '_|gettext|gettext_lazy|ngettext|ngettext_lazy'
+      - metavariable-regex:
+          metavariable: $STRING_ID
+          regex: ".*%\\w.*"
+    message: >
+      Do not use anonymous placeholders for translations.
+      Use printf style formatting with named placeholders instead.
+      For example, do `_("Hello %(name)s") % {"name": "Wagtail"}`
+      instead of `_("Hello %s") % "Wagtail"`.
+      See https://docs.wagtail.org/en/latest/contributing/translations.html#marking-strings-for-translation for more information.
+    languages: [python, javascript, typescript]
+    severity: ERROR
+  - id: translation-no-format-within-gettext-python
+    patterns:
+      - pattern: $FUNC("..." % ..., ...)
+      - metavariable-regex:
+          metavariable: $FUNC
+          regex: '_|gettext|gettext_lazy|ngettext|ngettext_lazy'
+    message: >
+      Do not format string before translations
+      or the interpolated value will be part of the key.
+      Instead, interpolate after the call to gettext.
+      For example, do `_("Hello %(name)s") % {"name": "Wagtail"}`
+      instead of `_("Hello %(name)s" % {"name": "Wagtail"} )`.
+      See https://docs.wagtail.org/en/latest/contributing/translations.html#marking-strings-for-translation for more information.
+    languages: [python]
+    severity: ERROR
+  - id: translation-no-format-within-gettext-javascript
+    patterns:
+      - pattern: $FUNC("...".replace(...), ...)
+      - metavariable-regex:
+          metavariable: $FUNC
+          regex: '_|gettext|gettext_lazy|ngettext|ngettext_lazy'
+    message: >
+      Do not format string before translations
+      or the interpolated value will be part of the key.
+      Instead, interpolate after the call to gettext.
+      For example, do `_("Hello %(name)s") % {"name": "Wagtail"}`
+      instead of `_("Hello %(name)s" % {"name": "Wagtail"} )`.
+      See https://docs.wagtail.org/en/latest/contributing/translations.html#marking-strings-for-translation for more information.
+    languages: [javascript, typescript]
+    severity: ERROR

Makefile

@@ -21,6 +21,7 @@ lint-server:
 	black --target-version py37 --check --diff .
 	flake8
 	isort --check-only --diff .
+	semgrep --config .semgrep.yml --error .
 	curlylint --parse-only wagtail
 	git ls-files '*.html' | xargs djhtml --check
 

client/src/components/Minimap/MinimapItem.tsx

@@ -36,10 +36,10 @@ const MinimapItem: React.FunctionComponent<MinimapItemProps> = ({
   const { href, label, icon, required, errorCount, level } = item;
   const hasError = errorCount > 0;
   const errorsLabel = ngettext(
-    '{num} error',
-    '{num} errors',
+    '%(num)s error',
+    '%(num)s errors',
     errorCount,
-  ).replace('{num}', `${errorCount}`);
+  ).replace('%(num)s', `${errorCount}`);
   const text = label.length > 26 ? `${label.substring(0, 26)}…` : label;
   return (
     <a

client/src/components/PageExplorer/PageExplorerItem.tsx

@@ -59,7 +59,7 @@ const PageExplorerItem: React.FunctionComponent<PageExplorerItemProps> = ({
       >
         <Icon
           name="edit"
-          title={gettext("Edit '{title}'").replace('{title}', title || '')}
+          title={gettext("Edit '%(title)s'").replace('%(title)s', title || '')}
           className="icon--item-action"
         />
       </Button>
@@ -72,8 +72,8 @@ const PageExplorerItem: React.FunctionComponent<PageExplorerItemProps> = ({
         >
           <Icon
             name="arrow-right"
-            title={gettext("View child pages of '{title}'").replace(
-              '{title}',
+            title={gettext("View child pages of '%(title)s'").replace(
+              '%(title)s',
               title || '',
             )}
             className="icon--item-action"

client/src/components/Sidebar/menu/SubMenuItem.tsx

@@ -97,8 +97,8 @@ export const SubMenuItem: React.FunctionComponent<SubMenuItemProps> = ({
               <span className="w-sr-only">
                 {dismissibleCount === 1
                   ? gettext('(1 new item in this menu)')
-                  : gettext('({number} new items in this menu)').replace(
-                      '{number}',
+                  : gettext('(%(number)s new items in this menu)').replace(
+                      '%(number)s',
                       `${dismissibleCount}`,
                     )}
               </span>

client/src/entrypoints/admin/page-editor.js

@@ -70,10 +70,10 @@ function initErrorDetection() {
       .find('[data-tabs-errors-statement]')
       .text(
         ngettext(
-          '({errorCount} error)',
-          '({errorCount} errors)',
+          '(%(errorCount)s error)',
+          '(%(errorCount)s errors)',
           errorCount,
-        ).replace('{errorCount}', errorCount),
+        ).replace('%(errorCount)s', errorCount),
       );
   });
 }

client/src/includes/bulk-actions.js

@@ -166,12 +166,12 @@ function onSelectIndividualCheckbox(e) {
       numObjectsSelected = getStringForListing('SINGULAR');
     } else if (numCheckedObjects === checkedState.numObjects) {
       numObjectsSelected = getStringForListing('ALL').replace(
-        '{0}',
+        '%(objects)s',
         numCheckedObjects,
       );
     } else {
       numObjectsSelected = getStringForListing('PLURAL').replace(
-        '{0}',
+        '%(objects)s',
         numCheckedObjects,
       );
     }

client/src/includes/sidePanel.js

@@ -133,10 +133,11 @@ export default function initSidePanel() {
       parseInt(Math.max(minWidth, Math.min(targetWidth, maxWidth)), 10) ||
       width;
 
-    const valueText = ngettext('{num} pixel', '{num} pixels', newWidth).replace(
-      '{num}',
+    const valueText = ngettext(
+      '%(num)s pixel',
+      '%(num)s pixels',
       newWidth,
-    );
+    ).replace('%(num)s', newWidth);
 
     sidePanelWrapper.style.width = `${newWidth}px`;
     widthInput.value = 100 - ((newWidth - minWidth) / range) * 100;

docs/contributing/general_guidelines.md

@@ -2,7 +2,7 @@
 
 ## Language
 
-British English is preferred for user-facing text; this text should also be marked for translation (using the `django.utils.translation.gettext` function and `{% trans %}` template tag, for example). However, identifiers within code should use American English if the British or international spelling would conflict with built-in language keywords; for example, CSS code should consistently use the spelling `color` to avoid inconsistencies like `background-color: $colour-red`.
+British English is preferred for user-facing text; this text should also be marked for translation (using the `django.utils.translation.gettext` function and `{% translate %}` template tag, for example). However, identifiers within code should use American English if the British or international spelling would conflict with built-in language keywords; for example, CSS code should consistently use the spelling `color` to avoid inconsistencies like `background-color: $colour-red`.
 
 ### Latin phrases and abbreviations
 

docs/contributing/translations.md

@@ -36,6 +36,38 @@ These new translations are imported into Wagtail for any subsequent RC and the f
 -   To translate a project, select it and enter your translation in the translation panel
 -   Save the translation using the translation button on the panel
 
+## Marking strings for translation
+
+In code, strings can be marked for translation with using Django's [translation system](django:topics/i18n/translation), using `gettext` or `gettext_lazy` in Python and `blocktranslate` and `translate` in templates.
+
+In both Python and templates, make sure to always use named placeholder. In addition, in Python, only use the printf style formatting. This is to ensure compatibility with Transifex and help translators in their work.
+
+For example:
+
+```python
+from django.utils.translation import gettext_lazy as _
+
+# Do this: printf style + named placeholders
+_("Page %(page_title)s with status %(status)s") % {"page_title": page.title, "status": page.status_string}
+
+# Do not use anonymous placeholders
+_("Page %s with status %s") % (page.title, page.status_string)
+_("Page {} with status {}").format(page.title, page.status_string)
+
+# Do not use positional placeholders
+_("Page {0} with status {1}").format(page.title, page.status_string)
+
+# Do not use new style
+_("Page {page_title} with status {status}").format(page_title=page.title, status=page.status_string)
+
+# Do not interpolate within the gettext call
+_("Page %(page_title)s with status %(status)s" % {"page_title": page.title, "status": page.status_string})
+_("Page {page_title} with status {status}".format(page_title=page.title, status=page.status_string))
+
+# Do not use f-string
+_(f"Page {page.title} with status {page.status_string}")
+```
+
 ## Additional resources
 
 -   [](django:topics/i18n/translation)

docs/contributing/ui_guidelines.md

@@ -46,4 +46,4 @@ We use [Prettier](https://prettier.io/) for formatting and [ESLint](https://esli
 
 This is an area of active improvement for Wagtail, with [ongoing discussions](https://github.com/wagtail/wagtail/discussions/8017).
 
--   Always use the `trimmed` attribute on `blocktrans` tags to prevent unnecessary whitespace from being added to the translation strings.
+-   Always use the `trimmed` attribute on `blocktranslate` tags to prevent unnecessary whitespace from being added to the translation strings.

docs/extending/custom_bulk_actions.md

@@ -44,7 +44,7 @@ An example of a confirmation template is as follows:
 {% extends 'wagtailadmin/bulk_actions/confirmation/base.html' %}
 {% load i18n wagtailadmin_tags %}
 
-{% block titletag %}{% blocktrans trimmed count counter=items|length %}Delete 1 item{% plural %}Delete {{ counter }} items{% endblocktrans %}{% endblock %}
+{% block titletag %}{% blocktranslate trimmed count counter=items|length %}Delete 1 item{% plural %}Delete {{ counter }} items{% endblocktranslate %}{% endblock %}
 
 {% block header %}
     {% trans "Delete" as del_str %}
@@ -66,7 +66,7 @@ An example of a confirmation template is as follows:
 
 {% block items_with_no_access %}
 
-{% blocktrans trimmed asvar no_access_msg count counter=items_with_no_access|length %}You don't have permission to delete this item{% plural %}You don't have permission to delete these items{% endblocktrans %}
+{% blocktranslate trimmed asvar no_access_msg count counter=items_with_no_access|length %}You don't have permission to delete this item{% plural %}You don't have permission to delete these items{% endblocktranslate %}
 {% include './list_items_with_no_access.html' with items=items_with_no_access no_access_msg=no_access_msg %}
 
 {% endblock items_with_no_access %}

setup.py

@@ -60,6 +60,7 @@
     "flake8-print==5.0.0",
     "doc8==0.8.1",
     "flake8-assertive==2.0.0",
+    "semgrep",
     # For templates linting
     "curlylint==0.13.1",
     # For template indenting

wagtail/admin/action_menu.py

@@ -122,13 +122,15 @@ def get_context_data(self, parent_context):
             workflow_state
             and workflow_state.status == workflow_state.STATUS_NEEDS_CHANGES
         ):
-            context["label"] = _("Resubmit to {}").format(
-                workflow_state.current_task_state.task.name
-            )
+            context["label"] = _("Resubmit to %(task_name)s") % {
+                "task_name": workflow_state.current_task_state.task.name
+            }
         elif page:
             workflow = page.get_workflow()
             if workflow:
-                context["label"] = _("Submit to {}").format(workflow.name)
+                context["label"] = _("Submit to %(workflow_name)s") % {
+                    "workflow_name": workflow.name
+                }
         return context
 
 

wagtail/admin/forms/auth.py

@@ -20,9 +20,9 @@ class LoginForm(AuthenticationForm):
 
     def __init__(self, request=None, *args, **kwargs):
         super().__init__(request=request, *args, **kwargs)
-        self.fields["username"].widget.attrs["placeholder"] = (
-            gettext_lazy("Enter your %s") % self.username_field.verbose_name
-        )
+        self.fields["username"].widget.attrs["placeholder"] = gettext_lazy(
+            "Enter your %(username_field_name)s"
+        ) % {"username_field_name": self.username_field.verbose_name}
         self.fields["username"].widget.attrs["autofocus"] = ""
 
     @property

wagtail/admin/forms/pages.py

@@ -101,9 +101,9 @@ def clean(self):
             self._errors["new_slug"] = self.error_class(
                 [
                     _(
-                        'This slug is already in use within the context of its parent page "%s"'
+                        'This slug is already in use within the context of its parent page "%(parent_page_title)s"'
                     )
-                    % parent_page
+                    % {"parent_page_title": parent_page}
                 ]
             )
             # The slug is no longer valid, hence remove it from cleaned_data

wagtail/admin/forms/tags.py

@@ -45,13 +45,11 @@ def clean(self, value):
                 value_too_long += val
         if value_too_long:
             raise ValidationError(
-                _(
-                    "Tag(s) %(value_too_long)s are over %(max_tag_length)d characters"
-                    % {
-                        "value_too_long": value_too_long,
-                        "max_tag_length": max_tag_length,
-                    }
-                )
+                _("Tag(s) %(value_too_long)s are over %(max_tag_length)d characters")
+                % {
+                    "value_too_long": value_too_long,
+                    "max_tag_length": max_tag_length,
+                }
             )
 
         if not self.free_tagging:

wagtail/admin/forms/workflows.py

@@ -96,9 +96,10 @@ def clean(self):
                 self.add_error(
                     "page",
                     ValidationError(
-                        _("This page already has workflow '{0}' assigned.").format(
-                            existing_workflow
-                        ),
+                        _(
+                            "This page already has workflow '%(workflow_name)s' assigned."
+                        )
+                        % {"workflow_name": existing_workflow},
                         code="existing_workflow",
                     ),
                 )