Problem/Motivation

t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead

Fix classes:
core/modules/config/src/Form/ConfigSync.php
core/modules/field_ui/src/Form/EntityFormDisplayEditForm.php
core/modules/field_ui/src/Form/EntityViewDisplayEditForm.php
core/modules/image/src/Form/ImageStyleEditForm.php
core/modules/menu_ui/src/Form/MenuDeleteForm.php
core/modules/node/src/Form/NodeRevisionDeleteForm.php
core/modules/node/src/Form/NodeRevisionRevertForm.php
core/modules/node/src/Form/NodeRevisionRevertTranslationForm.php
core/modules/node/src/Form/RebuildPermissionsForm.php
core/modules/quickedit/src/Form/QuickEditFieldForm.php
core/modules/shortcut/src/Form/SetCustomize.php
core/modules/shortcut/src/Form/ShortcutSetDeleteForm.php
core/modules/system/src/Form/CronForm.php
core/modules/system/src/Form/DateFormatAddForm.php
core/modules/system/src/Form/DateFormatDeleteForm.php
core/modules/system/src/Form/DateFormatEditForm.php
core/modules/system/src/Form/DateFormatFormBase.php
core/modules/system/src/Form/FileSystemForm.php
core/modules/system/src/Form/LoggingForm.php
core/modules/system/src/Form/PerformanceForm.php
core/modules/system/src/Form/RegionalForm.php
core/modules/system/src/Form/RssFeedsForm.php
core/modules/system/src/Form/SiteInformationForm.php
core/modules/system/src/Form/SiteMaintenanceModeForm.php
core/modules/system/src/Form/SystemBrandingOffCanvasForm.php
core/modules/system/src/Form/ThemeSettingsForm.php
core/modules/views/src/Form/ViewsFormMainForm.php
core/modules/views_ui/src/Form/Ajax/EditDetails.php
core/modules/views_ui/src/Form/Ajax/Rearrange.php
core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
core/modules/views_ui/src/Form/BasicSettingsForm.php

CommentFileSizeAuthor
#70 reroll_diff_63-70.txt5.09 KBraman.b
#70 3122912-70.patch87.84 KBraman.b
#64 interdiff_59_63.txt10.47 KBKapilV
#63 3122912-63.patch88.73 KBKapilV
#61 Review-4.PNG29.48 KBSivaprasadC
#61 Review-3.PNG4.29 KBSivaprasadC
#61 Review-2.PNG89.23 KBSivaprasadC
#61 Review-1.PNG79.61 KBSivaprasadC
#59 interdiff_54-58.txt24.94 KBsiddhant.bhosale
#59 3122912-58.patch88.75 KBsiddhant.bhosale
#54 interdiff_37-54.txt6.97 KBsiddhant.bhosale
#54 3122912-54.patch62.13 KBsiddhant.bhosale
#52 interdiff_37-52.txt118.03 KBsiddhant.bhosale
#52 3122912-52.patch71.87 KBsiddhant.bhosale
#47 interdiff-37-46.txt6.27 KBS_Bhandari
#46 interdiff-37-46.patch6.27 KBS_Bhandari
#46 3122912-46.patch61.21 KBS_Bhandari
#42 interdiff-37-42.txt52.6 KBS_Bhandari
#42 3122912-42.patch7.06 KBS_Bhandari
#37 t_calls_dependency_injection_in_Form_Builders-3122912-37.patch54.6 KBpaulocs
#35 interdiff-3122912-29-34.txt771 bytesmartin107
#35 3122912-34.patch54.93 KBmartin107
#33 3122912-interdiff-15-19-29.txt7.83 KBshaktik
#29 3122912-29.patch54.48 KBshaktik
#25 fixing-more-testcase-3122912-25.patch55.09 KBshaktik
#22 fixing-test-cases-3122912-22.patch65.47 KBshaktik
#19 3122912-interdiff-15-19.txt11.53 KBshaktik
#19 3122912-19.patch66.37 KBshaktik
#16 interdiff-12-15-3122912.txt626 bytesmartin107
#15 3122912-15.patch62.41 KBmartin107
#12 fixing-test-case-3122912-12.patch62.05 KBshaktik
#8 3122912-7.patch55.41 KBshaktik
#4 interdiff_2-4.txt723 bytesswatichouhan012
#4 3122912-4.patch56.24 KBswatichouhan012
#2 3122912-2.patch56.24 KBswatichouhan012
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swatichouhan012 created an issue. See original summary.

swatichouhan012’s picture

Assigned: swatichouhan012 » Unassigned
Status: Active » Needs review
Parent issue: » #3113904: [META] Replace t() calls inside of classes
FileSize
56.24 KB

Wrt to comment #15 in #3113904: [META] Replace t() calls inside of classes i checked with all forms to fix t() calls, kindly review patch file

Status: Needs review » Needs work

The last submitted patch, 2: 3122912-2.patch, failed testing. View results

swatichouhan012’s picture

Status: Needs work » Needs review
FileSize
56.24 KB
723 bytes

Fixed type issue in patch

Status: Needs review » Needs work

The last submitted patch, 4: 3122912-4.patch, failed testing. View results

plach’s picture

Version: 9.0.x-dev » 9.1.x-dev

This kind of changes should target 9.1.x now, thanks!

shaktik’s picture

Assigned: Unassigned » shaktik
Status: Needs work » Active
shaktik’s picture

Fixed t() cells dependency injection in Form, kindly review.

Thanks,
Shakti

shaktik’s picture

Status: Active » Needs review
shaktik’s picture

Assigned: shaktik » Unassigned

Status: Needs review » Needs work

The last submitted patch, 8: 3122912-7.patch, failed testing. View results

shaktik’s picture

shaktik’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: fixing-test-case-3122912-12.patch, failed testing. View results

martin107’s picture

Status: Needs work » Needs review
FileSize
62.41 KB

This should reduce the error count.. a little.

martin107’s picture

Status: Needs review » Needs work

The last submitted patch, 15: 3122912-15.patch, failed testing. View results

shaktik’s picture

Hi @martin107,

I am checking another test case.

shaktik’s picture

shaktik’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: 3122912-19.patch, failed testing. View results

shaktik’s picture

shaktik’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: fixing-test-cases-3122912-22.patch, failed testing. View results

shaktik’s picture

shaktik’s picture

Status: Needs work » Needs review
quietone’s picture

+++ b/core/modules/image/src/Form/ImageStyleEditForm.php
@@ -253,8 +253,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-    parent::save($form, $form_state);
...
+    parent::save($form, $form_state);

Why is this changed? It seems unrelated to t().

Status: Needs review » Needs work

The last submitted patch, 25: fixing-more-testcase-3122912-25.patch, failed testing. View results

shaktik’s picture

Status: Needs work » Needs review
FileSize
54.48 KB

Status: Needs review » Needs work

The last submitted patch, 29: 3122912-29.patch, failed testing. View results

shaktik’s picture

Assigned: Unassigned » shaktik
martin107’s picture

shaktik -- Can I ask you to produce an interdiff while you work... it is much easier for us to follow along and give suggestions.

Here is some documentation on how to produce one.

https://www.drupal.org/documentation/git/interdiff

shaktik’s picture

Hi @ martin,

please check attached interdiff, I am trying to fix rest 6 error test failer.

shaktik’s picture

Assigned: shaktik » Unassigned
martin107’s picture

Status: Needs work » Needs review
FileSize
54.93 KB
771 bytes

Look for the test with the shortest run time.. ConfigAccessTest

Reading the test class I saw it was looking for a particular block.

$block = $this->placeBlock('system_branding_block');

The system_branding_block has the following annotation

/**
 * Provides a block to display 'Site branding' elements.
 *
 * @Block(
 *   id = "system_branding_block",
 *   admin_label = @Translation("Site branding"),
 *   forms = {
 *     "settings_tray" = "Drupal\system\Form\SystemBrandingOffCanvasForm",
 *   },
 * )
 */

and that told me where to look ...

That SystemBrandingOffCanvasForm was missing the StringTranslationTrait

I hope that explanation helps.

paulocs’s picture

Status: Needs review » Needs work

Patch needs re-roll

paulocs’s picture

New patch.

longwave’s picture

Can someone write an issue summary so we understand what is being fixed here and can discuss the scope of the issue please?

paulocs’s picture

Title: t() calls dependency injection in Form Builders » t() calls should be avoided , use $this->t() instead in Form Builders
Issue summary: View changes
Issue tags: -Needs issue summary update

Updating issue summary.

longwave’s picture

Status: Needs review » Needs work

Thanks! The patch is looking good but I found a few more cases that weren't covered:

core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php:      '#title' => t('Show as expanded'),
core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php:      $this->messenger()->addWarning(t('All necessary changes to %dir and %file have been made, so you should remove write permissions to them now in order to avoid security risks. If you are unsure how to do so, consult the <a href=":handbook_url">online handbook</a>.', ['%dir' => $settings_dir, '%file' => $settings_file, ':handbook_url' => 'https://www.drupal.org/server-permissions']));
core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php:      'title' => t('Deleting @entity_type_plural', [
core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php:      $context['message'] = t('Deleting items... Completed @percentage% (@current of @total).', ['@percentage' => round(100 * $context['sandbox']['progress'] / $context['sandbox']['max']), '@current' => $context['sandbox']['progress'], '@total' => $context['sandbox']['max']]);
core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php:    \Drupal::messenger()->addStatus(t('All @entity_type_plural have been deleted.', ['@entity_type_plural' => $entity_type_plural]));
core/modules/quickedit/src/Form/QuickEditFieldForm.php:      $entity->revision_log = t('Updated the %field-name field through in-place editing.', ['%field-name' => $entity->get($field_name)->getFieldDefinition()->getLabel()]);
core/modules/config_translation/src/Form/ConfigTranslationFormBase.php:        $definition->setLabel(t('n/a'));
core/modules/views_ui/src/Form/Ajax/ReorderDisplays.php:          '#title' => t('Remove @id', ['@id' => $id]),
S_Bhandari’s picture

Hi,

I am working on it. Will update as soon as possible.

Thanks.

S_Bhandari’s picture

Status: Needs work » Needs review
FileSize
7.06 KB
52.6 KB

Hi,

Added a patch for the remaining ones. Please review the same and let me know for any observations.

Thanks.

longwave’s picture

Status: Needs review » Needs work

The patch is missing the previous changes, the interdiff shows them being removed.

S_Bhandari’s picture

Status: Needs work » Needs review

Hi @longwave,

Thanks for reviewing it. I will add the interdiff and the patch on top of the last patch submitted.

Thanks.

S_Bhandari’s picture

Status: Needs review » Needs work
S_Bhandari’s picture

Status: Needs work » Needs review
FileSize
61.21 KB
6.27 KB

Hi @longwave,

Added the new patch and inerdiff on top of #37. Please review the same and let me know for any other observations.

Thanks.

S_Bhandari’s picture

FileSize
6.27 KB

Hi,

Mistakenly added the interdiff with the '.patch' extension. Adding the right one.

Thanks.

The last submitted patch, 42: 3122912-42.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 46: 3122912-46.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 46: interdiff-37-46.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale
siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
Status: Needs work » Needs review
FileSize
71.87 KB
118.03 KB

Hi, I have made the changes over the patch in comment #37 and changes asked by @longwave in comment #40. Adding the patch as well as interdiff.
Please ignore this patch.

siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale
Status: Needs review » Needs work

I have added the wrong patch. Uploading the correct one.

siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
Status: Needs work » Needs review
FileSize
62.13 KB
6.97 KB

Hi, I have made the changes over the patch in comment #37 and changes asked by @longwave in comment #40. Adding the patch as well as interdiff.

paulocs’s picture

Status: Needs review » Reviewed & tested by the community

Patch #54 looks good to me!

Set to RTBC.

siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale
siddhant.bhosale’s picture

Status: Reviewed & tested by the community » Needs review

These are instances where that weren't covered

core/modules/block_content/src/BlockContentTypeForm.php:      '#title' => t('Label')

core/modules/block_content/src/BlockContentTypeForm.php:      '#description' => t("Provide a label for this block type to help identify it in the administration pages."),


core/modules/block_content/src/BlockContentTypeForm.php:      '#description' => t('Enter a description for this block type.'),


core/modules/block_content/src/BlockContentTypeForm.php:      '#title' => t('Description'),


core/modules/block_content/src/BlockContentTypeForm.php:      '#title' => t('Create new revision'),

core/modules/block_content/src/BlockContentTypeForm.php:      '#description' => t('Create a new revision by default for this block type.'),

core/modules/block_content/src/BlockContentTypeForm.php:        '#title' => t('Language settings'),

core/modules/block_content/src/BlockContentTypeForm.php:      '#value' => t('Save'),

core/modules/comment/src/CommentTypeForm.php:      '#title' => t('Label'),

core/modules/comment/src/CommentTypeForm.php:      '#description' => t('Describe this comment type. The text will be displayed on the <em>Comment types</em> administration overview page.'),

core/modules/comment/src/CommentTypeForm.php:      '#title' => t('Description'),

core/modules/comment/src/CommentTypeForm.php:        '#title' => t('Target entity type'),

core/modules/comment/src/CommentTypeForm.php:        '#description' => t('The target entity type can not be changed after the comment type has been created.'),

core/modules/comment/src/CommentTypeForm.php:        '#title' => t('Target entity type'),

core/modules/comment/src/CommentTypeForm.php:        '#title' => t('Language settings'),

core/modules/comment/src/CommentTypeForm.php:      '#value' => t('Save'),

core/modules/menu_ui/src/MenuForm.php:      '#title' => t('Administrative summary'),

core/modules/menu_ui/src/MenuForm.php:      '#title' => t('Menu language'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Name'),

core/modules/node/src/NodeTypeForm.php:      '#description' => t('The human-readable name of this content type. This text will be displayed as part of the list on the <em>Add content</em> page. This name must be unique.'),

core/modules/node/src/NodeTypeForm.php:      '#description' => t('A unique machine-readable name for this content type. It must only contain lowercase letters, numbers, and unders
cores. This name will be used for constructing the URL of the %node-add page, in which unders
cores will be converted into hyphens.', [

core/modules/node/src/NodeTypeForm.php:        '%node-add' => t('Add content'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Description'),

core/modules/node/src/NodeTypeForm.php:      '#description' => t('This text will be displayed on the <em>Add new content</em> page.'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Submission form settings'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Title field label'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Preview before submitting'),

core/modules/node/src/NodeTypeForm.php:        DRUPAL_DISABLED => t('Disabled'),

core/modules/node/src/NodeTypeForm.php:        DRUPAL_OPTIONAL => t('Optional'),

core/modules/node/src/NodeTypeForm.php:        DRUPAL_REQUIRED => t('Required'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Explanation or submission guidelines'),

core/modules/node/src/NodeTypeForm.php:      '#description' => t('This text will be displayed at the top of the page when creating or editing content of this type.'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Publishing options'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Default options'),

core/modules/node/src/NodeTypeForm.php:        'status' => t('Published'),

core/modules/node/src/NodeTypeForm.php:        'promote' => t('Promoted to front page'),

core/modules/node/src/NodeTypeForm.php:        'sticky' => t('Sticky at top of lists'),

core/modules/node/src/NodeTypeForm.php:        'revision' => t('Create new revision'),

core/modules/node/src/NodeTypeForm.php:      '#description' => t('Users with sufficient access rights will be able to override these options.'),

core/modules/node/src/NodeTypeForm.php:        '#title' => t('Language settings'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Display settings'),

core/modules/node/src/NodeTypeForm.php:      '#title' => t('Display author and date information'),

core/modules/node/src/NodeTypeForm.php:      '#description' => t('Author username and publish date will be displayed.'),

core/modules/node/src/NodeTypeForm.php:    $actions['submit']['#value'] = t('Save content type');

core/modules/node/src/NodeForm.php:      '#title' => t('Authoring information'),

core/modules/node/src/NodeForm.php:      '#title' => t('Promotion options'),

core/modules/node/src/NodeForm.php:      '#value' => t('Preview'),

core/modules/responsive_image/src/ResponsiveImageStyleForm.php:      '#description' => t('Select the smallest image style you expect to appear in this space. The fallback image style should only appear on the site if an error occurs.'),

core/modules/shortcut/src/ShortcutSetForm.php:      '#title' => t('Set name'),

core/modules/shortcut/src/ShortcutSetForm.php:      '#description' => t('The new set is created by copying links from your default shortcut set.'),

core/modules/shortcut/src/ShortcutSetForm.php:    $form['actions']['submit']['#value'] = t('Create new set');

core/modules/statistics/src/StatisticsSettingsForm.php:      '#title' => t('Content viewing counter settings'),

core/modules/statistics/src/StatisticsSettingsForm.php:      '#title' => t('Count content views'),

core/modules/statistics/src/StatisticsSettingsForm.php:      '#description' => t('Increment a counter each time content is viewed.'),

core/modules/update/src/UpdateSettingsForm.php:      '#title' => t('Check for updates'),

core/modules/update/src/UpdateSettingsForm.php:        '1' => t('Daily'),

core/modules/update/src/UpdateSettingsForm.php:        '7' => t('Weekly'),

core/modules/update/src/UpdateSettingsForm.php:      '#description' => t('Select how frequently you want to automatically check for new releases of your currently installed modules and themes.'),

core/modules/update/src/UpdateSettingsForm.php:      '#title' => t('Check for updates of uninstalled modules and themes'),

core/modules/update/src/UpdateSettingsForm.php:      '#title' => t('Email addresses to notify when updates are available'),

core/modules/update/src/UpdateSettingsForm.php:      '#description' => t('Whenever your site checks for available updates and finds new releases, it can notify a list of users via email. Put each address on a separate line. If blank, no emails will be sent.'),

core/modules/update/src/UpdateSettingsForm.php:      '#title' => t('Email notification threshold'),

core/modules/update/src/UpdateSettingsForm.php:        'all' => t('All newer versions'),

core/modules/update/src/UpdateSettingsForm.php:        'security' => t('Only security updates'),

core/modules/update/src/UpdateSettingsForm.php:      '#description' => t('You can choose to send email only if a security update is available, or to be notified about all newer versions. If there are updates available of Drupal 
core or any of your installed modules and themes, your site will always print a message on the <a href=":status_report">status report</a> page, and will also display an error message on administration pages if there is a security update.', [':status_report' => Url::fromRoute('system.status')->toString()]),

core/modules/user/src/AccountForm.php:      '#title' => t('Locale settings'),

core/modules/user/src/AccountForm.php:      '#title' => t('Time zone'),

core/modules/user/src/AccountForm.php:      '#description' => t('Select the desired local time and time zone. Dates and times throughout this site will be displayed using this time zone.'),

core/modules/views_ui/src/ViewAddForm.php:      '#title' => t('View basic information'),

core/modules/views_ui/src/ViewAddForm.php:      '#title' => t('View settings'),
longwave’s picture

Status: Needs review » Needs work

Oh, well spotted, my search wasn't good enough to find those forms. Needs work to include those changes too.

siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
Status: Needs work » Needs review
FileSize
88.75 KB
24.94 KB

Hi, I have uploaded the patch and interdiff as well. In this patch only the instances in the forms are present. Have removed the instances where testForms were covered. We will have another issue for that. Please review.

SivaprasadC’s picture

Assigned: Unassigned » SivaprasadC
SivaprasadC’s picture

Assigned: SivaprasadC » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
79.61 KB
89.23 KB
4.29 KB
29.48 KB

Thanks @siddhant.bhosale for the Patch #59, looks good to me!

Aplied the patch successfully, and cross verified with code sniffer for any missed files. Seems all covered.

Kindly refer to the attached. Set to RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

@SivaprasadC: The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that. The same goes for coding standards sniffs. Neither are also not sufficient criteria for the issue to be marked "Reviewed and Tested by the Community".

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

The patch is currently not applying to 9.1.x, so that at least needs to be fixed.

KapilV’s picture

Status: Needs work » Needs review
FileSize
88.73 KB

Hear Updated Patch.

KapilV’s picture

FileSize
10.47 KB

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me and seems to cover all non-test forms, I can't find any remaining instances.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: 3122912-63.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Double random fail, as far as I can see.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll for 9.2.x

raman.b’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
87.84 KB
5.09 KB

Re-rolled for 9.2.x

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 70: 3122912-70.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Random fail:

Drupal\Core\Database\SchemaObjectExistsException: Table 'sequences' already exists.

  • catch committed 0b1816c on 9.2.x
    Issue #3122912 by shaktik, siddhant.bhosale, S_Bhandari, martin107,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0b1816c and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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

smustgrave credited Mile23.

smustgrave’s picture