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
Comment | File | Size | Author |
---|---|---|---|
#70 | reroll_diff_63-70.txt | 5.09 KB | raman.b |
#70 | 3122912-70.patch | 87.84 KB | raman.b |
#64 | interdiff_59_63.txt | 10.47 KB | KapilV |
#63 | 3122912-63.patch | 88.73 KB | KapilV |
#61 | Review-4.PNG | 29.48 KB | SivaprasadC |
Comments
Comment #2
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedWrt 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
Comment #4
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedFixed type issue in patch
Comment #6
plachThis kind of changes should target 9.1.x now, thanks!
Comment #7
shaktikComment #8
shaktikFixed t() cells dependency injection in Form, kindly review.
Thanks,
Shakti
Comment #9
shaktikComment #10
shaktikComment #12
shaktikComment #13
shaktikComment #15
martin107 CreditAttribution: martin107 as a volunteer commentedThis should reduce the error count.. a little.
Comment #16
martin107 CreditAttribution: martin107 as a volunteer commentedComment #18
shaktikHi @martin107,
I am checking another test case.
Comment #19
shaktikComment #20
shaktikComment #22
shaktikComment #23
shaktikComment #25
shaktikComment #26
shaktikComment #27
quietone CreditAttribution: quietone as a volunteer commentedWhy is this changed? It seems unrelated to t().
Comment #29
shaktikComment #31
shaktikComment #32
martin107 CreditAttribution: martin107 as a volunteer commentedshaktik -- 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
Comment #33
shaktikHi @ martin,
please check attached interdiff, I am trying to fix rest 6 error test failer.
Comment #34
shaktikComment #35
martin107 CreditAttribution: martin107 as a volunteer commentedLook 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
and that told me where to look ...
That SystemBrandingOffCanvasForm was missing the StringTranslationTrait
I hope that explanation helps.
Comment #36
paulocsPatch needs re-roll
Comment #37
paulocsNew patch.
Comment #38
longwaveCan someone write an issue summary so we understand what is being fixed here and can discuss the scope of the issue please?
Comment #39
paulocsUpdating issue summary.
Comment #40
longwaveThanks! The patch is looking good but I found a few more cases that weren't covered:
Comment #41
S_Bhandari CreditAttribution: S_Bhandari as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi,
I am working on it. Will update as soon as possible.
Thanks.
Comment #42
S_Bhandari CreditAttribution: S_Bhandari as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi,
Added a patch for the remaining ones. Please review the same and let me know for any observations.
Thanks.
Comment #43
longwaveThe patch is missing the previous changes, the interdiff shows them being removed.
Comment #44
S_Bhandari CreditAttribution: S_Bhandari as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @longwave,
Thanks for reviewing it. I will add the interdiff and the patch on top of the last patch submitted.
Thanks.
Comment #45
S_Bhandari CreditAttribution: S_Bhandari as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #46
S_Bhandari CreditAttribution: S_Bhandari as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @longwave,
Added the new patch and inerdiff on top of #37. Please review the same and let me know for any other observations.
Thanks.
Comment #47
S_Bhandari CreditAttribution: S_Bhandari as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi,
Mistakenly added the interdiff with the '.patch' extension. Adding the right one.
Thanks.
Comment #51
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #52
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedHi, 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.
Comment #53
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedI have added the wrong patch. Uploading the correct one.
Comment #54
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedHi, 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.
Comment #55
paulocsPatch #54 looks good to me!
Set to RTBC.
Comment #56
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #57
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedThese are instances where that weren't covered
Comment #58
longwaveOh, well spotted, my search wasn't good enough to find those forms. Needs work to include those changes too.
Comment #59
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedHi, 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.
Comment #60
SivaprasadC CreditAttribution: SivaprasadC as a volunteer and at Tech Mahindra for Drupal India Association commentedComment #61
SivaprasadC CreditAttribution: SivaprasadC as a volunteer and at Tech Mahindra for Drupal India Association commentedThanks @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.
Comment #62
xjm@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.
Comment #63
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedHear Updated Patch.
Comment #64
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedComment #66
longwaveThis looks good to me and seems to cover all non-test forms, I can't find any remaining instances.
Comment #68
longwaveDouble random fail, as far as I can see.
Comment #69
catchNeeds a re-roll for 9.2.x
Comment #70
raman.b CreditAttribution: raman.b at OpenSense Labs commentedRe-rolled for 9.2.x
Comment #72
longwaveRandom fail:
Comment #74
catchCommitted 0b1816c and pushed to 9.2.x. Thanks!
Comment #80
smustgrave CreditAttribution: smustgrave at Mobomo commentedClosed #2806983: Replace non-test usage of t() with injected service as a duplicate of this.