Problem/Motivation

I was reviewing the code of SystemController.php and noticed that there was a procedural t() function instead of $this->t() which is not a best practice to use inside a controller.

$admin_theme_options[$theme->getName()] = $theme->info['name'] . ($theme->isExperimental() ? ' (' . t('Experimental') . ')' : '');

Steps to reproduce

Proposed resolution

Fix Controllers in non tests for the sniff 'Drupal.Commenting.DocComment.ShortSingleLine'
That should be these files

  • core/modules/views_ui/src/Controller/ViewsUIController.php
  • core/modules/book/src/Controller/BookController.php Moved to contrib
  • core/modules/tracker/src/Controller/TrackerController.php Moved to contrib
  • core/modules/system/src/Controller/SystemController.php
  • core/modules/update/src/Controller/UpdateController.php

Remaining tasks

review
commit

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3314260

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

rakesh.drupal created an issue. See original summary.

rakesh.drupal’s picture

Assigned: rakesh.drupal » Unassigned
Status: Needs work » Needs review
StatusFileSize
new842 bytes
quietone’s picture

Component: system.module » other
Category: Bug report » Task
Status: Needs review » Needs work
Issue tags: +coding standard, +Needs issue rescope

@rakesh.drupal, thanks for the patch.

The change of t() to $this->t is being addressed in #3113904: [META] Replace t() calls inside of classes . It is not being done on an individual basis because there are too many instances and there is a coder sniff for this. For example, there was an issue to convert all the t() functions in plugins. #3181778: [w/c September 17th] Replace t() with $this->t() in all plugins. I think what should happen here is that this is re-scoped to cover, say all Controllers. That is only a suggestion, testing by changing the phpcs.xml rule as is shown in the Issue Summary of #3181778: [w/c September 17th] Replace t() with $this->t() in all plugins would help to determine a sensible scope. This can then be a child of the meta.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

avpaderno’s picture

Issue tags: -coding standard +Coding standards
kunalgautam’s picture

Status: Needs work » Needs review
StatusFileSize
new139.47 KB

Patch #2 is working fine for Drupal 10.1.x version as well.

avpaderno’s picture

Status: Needs review » Needs work

What requested in comment #3 hasn't been done. This means the status is still Needs work.

I think what should happen here is that this is re-scoped to cover, say all Controllers.

ravi.shankar’s picture

Working on this.

ravi.shankar’s picture

Status: Needs work » Needs review
StatusFileSize
new7.69 KB

Replaced all the t() with $this->t() in all the controllers.

Added a patch please review.

spokje’s picture

Status: Needs review » Needs work

Looks like not all Controllers that are in patch #9 use the StringTranslationTrait or extend a class that does.

At the very least: TestBot is not happy.

Also, if we go the replace-in-controller-only route in this issue, we need an update of the Issue Summary and title to make that clear.

chaitanyadessai’s picture

Status: Needs work » Needs review
StatusFileSize
new11.99 KB

Replaced all the t() with $this->t()
Please review.

avpaderno’s picture

Status: Needs review » Needs work
avpaderno’s picture

Also, the issue summary needs to be updated, since the issue has been rescoped.

quietone’s picture

Title: Best practice is $this->t() instead of t() in SystemController.php » Fix Controllers with trait in non tests for 'DrupalPractice.Objects.GlobalFunction'
Issue summary: View changes
Issue tags: -Needs issue rescope, -Needs issue summary update
Parent issue: » #3113904: [META] Replace t() calls inside of classes

I've been doing some more digging into the issues for fixing t() function. In this comment in the parent issue xjm outlines the scope for these issues. That means, that this issue needs to split into two issues, one for the Controllers that have the trait and one for the Controllers that do not have the trait.

There is also a sniff for this. To find the find that need to be changed I altered the DrupalPractice.Objects.GlobalFunction rule in phpcs.xml to the following.

  <rule ref="DrupalPractice.Objects.GlobalFunction">
    <include-pattern>*/Plugin/*</include-pattern>
    <include-pattern>*Controller*</include-pattern>
    <exclude-pattern>*/tests/*</exclude-pattern>
  </rule>

The results show errors in 8 files, 5 of which extend from ControllerBase, which has the trait. Lets do those here. I have updated the IS with the files to be fixed here.

The ones that do not have the trait are

  • core/modules/system/src/Controller/SystemInfoController.php
  • core/modules/file/src/Controller/FileWidgetAjaxController.php
  • core/modules/config/src/Controller/ConfigController.php

And are to be done in a separate issue.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Title: Fix Controllers with trait in non tests for 'DrupalPractice.Objects.GlobalFunction' » Replace t() calls inside of Controllers that do not use StringTranslationTrait
quietone’s picture

Status: Needs work » Needs review
quietone’s picture

Issue summary: View changes
smustgrave’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Hiding patches for clarity.

Updated the summary to note that 2 of the findings aren't needed since those moved to contrib.

Reviewing the current code changes in the MR and change seems good

catch’s picture

Status: Reviewed & tested by the community » Needs work

One comment on the MR.

avpaderno’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.83 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

avpaderno’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems straight forward to me and non disruptive

nod_’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed d2cf8e5268 to 11.x and ceeb8e0277 to 11.0.x and c550b71cd1 to 10.4.x and 0ce937d2f6 to 10.3.x. Thanks!

  • nod_ committed 0ce937d2 on 10.3.x
    Issue #3314260 by quietone, avpaderno, rakesh.drupal, ravi.shankar,...

  • nod_ committed c550b71c on 10.4.x
    Issue #3314260 by quietone, avpaderno, rakesh.drupal, ravi.shankar,...

  • nod_ committed ceeb8e02 on 11.0.x
    Issue #3314260 by quietone, avpaderno, rakesh.drupal, ravi.shankar,...

  • nod_ committed d2cf8e52 on 11.x
    Issue #3314260 by quietone, avpaderno, rakesh.drupal, ravi.shankar,...
nod_’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.