Review phpcs reported use of translation for variable

Created on 4 May 2023, over 1 year ago
Updated 5 May 2023, over 1 year ago

Problem/Motivation

While most phpcs issues are being fixed by 📌 Fix the issues reported by phpcs Needs review , I feel this one requires special attention:

FILE: ...evelopment/ddev/accessbyref-3357621/web/modules/contrib/access_by_ref/src/Controller/AbrconfigListBuilder.php
-------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------
78 | WARNING | Only string literals should be passed to t() where possible
-------------------------------------------------------------------------------------------------------------------

This is caused by the ABR Config page, which displays help text on how to use the module.

Steps to reproduce

Checkout the 3357621-phpcs-fixes branch.

Execute the command: phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,js,info,txt,md,yml,twig web/modules/contrib/access_by_ref/

Proposed resolution

There are several approaches that could be taken:

  • Ignore the warning
    We could decide a variable is the best way to deal with this text and ignore this warning. If we do this, we could prefix the line with // phpcs:ignore
  • Change to a single string
    The we could combine the concatenated string into a single multi-line string, possibly using PHP's <<< END syntax.
  • Use multiple t() calls
    Instead of a single translate call on the whole message, we could break into individual paragraphs and translate each separately.
  • Move text out of the code
    We could take the text out of the class, and specify a template for the render array. We could then move the text into a .twig template. Text in .twig can be translated with {% trans %} tags.

Having this much text in a PHP class seems sub-optimal to me, so moving to a template would seem a cleaner approach to me, but I'd like to hear opinions before I implement.

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Active

Version

3.0

Component

Code

Created by

🇮🇪Ireland lostcarpark

Live updates comments and jobs are added and updated live.
  • Coding standards

    It involves compliance with, or the content of coding standards. Requires broad community agreement.

Sign in to follow issues

Comments & Activities

  • Issue created by @lostcarpark
  • 🇮🇪Ireland lostcarpark

    I discussed this at the DrupalEasy Office Hours, and we looked at some core modules for comparison.

    The general consensus was that it's okay to use formatting tags such as <em> and <strong> in t() calls, but structural tags such as <p> and <li> should be avoided. The examples we found (for example, the Actions module) kept the structural elements outside the translated strings.

    An advantage of this approach is that if the source text changes (for example, when adding a new reference type), the whole string will no longer match, and have to be replaced. When it's translated as a translation string per line, only the new string needs to be added, and the existing ones will be unaffected.

Production build 0.71.5 2024