Problem/Motivation
When attempting to unit test a widget which extends the StringTextfieldWidget the traditional method of mocking the string translation service fails and an error is produced Call to undefined function Drupal\Core\Field\Plugin\Field\FieldWidget\t().
On further inspection many of the plugins extending PluginBase do not utilize $this when calling t().
Steps to reproduce
1. Extend StringTextfieldWidget
1. Override settingsForm by adding to what parent produces
1. Unit test new class
Proposed resolution
PluginBase is using StringTranslationTrait but many of the child plugins are not utilizing it properly. Use $this->t() instead of t() for string translation calls for all classes extending PluginBase.
Enable phpcs rule, "DrupalPractice.Objects.GlobalFunction" for the pattern */Plugin/*.
<rule ref="DrupalPractice.Objects.GlobalFunction">
<include-pattern>*/Plugin/*</include-pattern>
</rule>Remaining tasks
Reviews
Keeping the patch up to date
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #114 | 3181778-114-9.5.x.patch | 222.16 KB | quietone |
| #114 | interdiff-111-114-9.5.x.txt | 705 bytes | quietone |
| #113 | 3181778-113-10.1.x.patch | 211.18 KB | quietone |
| #113 | interdiff-110-113-10.1.x.txt | 705 bytes | quietone |
| #111 | 3181778-111-9.5.x.patch | 222.07 KB | quietone |
Issue fork drupal-3181778
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
Comment #2
cchiste commentedComment #3
cchiste commentedThis patch swaps t() for $this->t() for the following widgets:
- BooleanCheckboxWidget
- EmailDefaultWidget
- EntityReferenceAutocompleteWidget
- NumberWidget
- OptionsButtonsWidget
- OptionsSelectWidget
- OptionsWidgetBase
- StringTextareaWidget
- StringTextfieldWidget
In addition to using new TranslatableMarkup() for the static OptionsWidgetBase::validateElement().
Comment #4
cchiste commentedComment #5
longwaveReplacing t() is an enormous task and scoping it effectively is tricky, but maybe grouping by plugin type is a good way of doing this.
However, there are many more FieldWidgets that need fixing up if we are to cover them all here, including:
./core/modules/comment/src/Plugin/Field/FieldWidget/CommentWidget.php
./core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDatelistWidget.php
./core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetMultiple.php
./core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidget.php
./core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
./core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
./core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
./core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
./core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
./core/modules/telephone/src/Plugin/Field/FieldWidget/TelephoneDefaultWidget.php
./core/modules/text/src/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php
Comment #6
longwaveWhile this causes problems with unit testing it isn't strictly a bug, so reclassifying as a task.
And I should have said earlier: thanks for working on this :)
Comment #7
longwaveIn fact, as Drupal\Core\Plugin\PluginBase includes StringTranslationTrait, I wonder if we should rescope this to cover all Field plugins or even *all* plugins that extend from PluginBase?
Comment #8
cchiste commentedComment #9
longwaveTried to automate this with:
Unsure if I missed any, let's see how this goes.
Comment #10
cchiste commentedGreat, thanks for that! I did find some D6/7 migration items (Derivers) that are not derivatives of class' using the StringTranslationTrait in the patch and have removed.
Comment #11
longwaveThanks! Rerolled as #10 didn't apply, and a few more fixes, hopefully the tests will complete now - I don't expect them all to pass but fingers crossed.
Comment #13
longwaveConverted calls in static methods to
new TranslatableMarkup()as we can't use $this in that context.Also removed some comments which didn't make sense any more:
t()just callsnew TranslatableMarkupanyway, so not sure what this is getting at, I think this must date to older code when t() returned translated strings directly.Comment #15
raman.b commentedAddressing the remaining failures
Comment #17
raman.b commentedA couple of more instances..
Comment #18
GrumpySchlagI review the patch #17 and found a new missing change on the class
Drupal\views\Plugin\views\field\FieldPluginBase.I run the command from #9 to found the new changes on
FieldPluginBase.Here is my changes and the interdiff.
PS: This is my first contribution to Drupal, I hope I didn't make too much mistake on my first patch.
Many thanks for your review
Comment #20
longwaveThanks for the patch, and welcome to the core issue queue!
The tests failed because this is in a static method, and we can't use $this in static context. Here we have to replace the call to
t()withnew TranslatableMarkup()instead.Comment #21
anushrikumari commentedComment #22
anushrikumari commentedImplemented the changes suggested in #20 by @longwave
Comment #23
anushrikumari commentedComment #24
longwaveNeeds a reroll.
Comment #25
longwaveComment #26
anushrikumari commentedComment #27
anushrikumari commentedrerolled the patch for 9.2.x
Comment #28
nishantghetiya commentedI applied #27 on drupal core 9.2.x and it applied successfully. I didn't find any other place where t() need to be replace in plugins.
Comment #29
longwave#27 fails a number of tests because $this is used in static context.
Comment #31
longwaveComment #32
renatog commentedThis patch contains long lines.
In this case for example. Please could you split this array into a second line? E.g.:
Line 62
From:
To:
Line 101.
From:
'maxMessage' => $this->t('%name: the value may be no greater than %max.', ['%name' => $label, '%max' => $max]),To:
And last but not least, array with many options. E.g.:
Line 99
From:
'#options' => ['MDY' => $this->t('Month/Day/Year'), 'DMY' => $this->t('Day/Month/Year'), 'YMD' => $this->t('Year/Month/Day')],To:
Is clear and easier to read the code
Comment #33
kapilv commentedComment #34
kapilv commentedAddressed #32
Comment #35
kapilv commentedComment #39
mallezieRerolled patch to 9.3.x.
Also reviewed the patch, and looks good to me. No unrelated changes, all changes look correct, and tests succeed.
Comment #40
vikashsoni commentedPatch working fine all changes looking good there is not issue after the patch
Thanks for the patch
Drupal version ---- 9.3.x.
Comment #41
longwaveI'm marking this RTBC based on the above two comments.
Comment #42
larowlanThis is great cleanup. Is there a way we can prevent these creeping back in with a phpcs rule?
Comment #43
alexpottSince #3151118: Include bootstrap.inc using composer this is less pressing. t() is present during unit tests now - we should have done that ages ago.
That said there's no harm in proceeding here but I think we should do this by trying to enable the existing PHPCS check for this. There is one - \DrupalPractice\Sniffs\Objects\GlobalFunctionSniff - I wonder how many of the other functions in are still existing - not too many as far as I can see - so this might work.
There are some out-of-scope changes and a question about whether or not we we translate test code.
Also I think we should try to do this during a beta release as this change affects many files and could disrupt other changes.
Comment #46
mauryarahul11Comment #48
vicheldt commentedI will do the review of this issue.
Comment #49
vicheldt commentedThere's still 2 files inside Drupal\Core\Field\Plugin\ that have t() instead of $this->t().
Comment #50
vicheldt commentedI changed the other 2 files that were using t() instead $this->t().
Comment #51
beatrizrodriguesI'll do the review
Comment #52
beatrizrodriguesReviewed. Looks good to me!
Comment #54
longwaveMerge request 1326 doesn't contain all the changes, I am not sure why this was opened separately and think this should be closed.
Merge request 1185 still has outstanding comments from @alexpott.
Comment #55
beatrizrodriguesI will apply the comments from @alexpott at MR 1185
Comment #56
beatrizrodriguesOh, I messed up the MR, I did the rebase from 9.4.x and not from the 9.3.x. Is there a way to change the merge target branch or do I have to open another MR? Sorry for the mess.
Comment #57
longwaveOnly the person who created the MR can change the target branch. If that wasn't you, then open a new MR I guess?
Comment #58
beatrizrodrigues@longwave thank you. I will do it.
Comment #60
beatrizrodriguesComment #61
jonas-gerosa361Reviewed MR !1680, looks good to me.
Comment #62
longwaveLots of test failures here.
Comment #63
libbna commentedhey @beatrizrodrigues are you still working on this issue? If not, I would like to work on this.
Comment #64
cchiste commentedComment #66
Aline Teixeira Ramos commentedHey, i can work on it! =)
Comment #68
Aline Teixeira Ramos commentedI applyed the changes from MR 1680 and created a new MR against the 9.5.x branch. Lets wait for the tests.
Comment #69
Aline Teixeira Ramos commentedHi guys. I made the necessary changes according to what is asked in the problem. After the changes, I didn't identify any errors related to the topic (change t() to $this->t() or new TranslatableMarkup). =)
Comment #70
Aline Teixeira Ramos commentedComment #71
Aline Teixeira Ramos commentedResubmitting with phpcbf corrections from comment #69.
Comment #72
rakesh.regarTried applying patch #70 but getting PHP fatal error
fixed some things that made it working on 9.5.x with php8.1 & php7.4
Comment #73
aldairsoares commentedThe last patch from @rakesh.regar passed the tests, thus I am going to review it.
Comment #74
aldairsoares commentedI applied the last patch at #72 comment and run phpcs to look for any mistake.
There is no phpcs erros and the task asked in this issue (use $this->t() instead of t()) is completed.
I left a screenshot to show the changes in one of the files, making a comparison with the patch.
I'm moving it to RTBC.
Comment #75
alexpott@aldairsoares thank you for looking into this issue.
Posting screenshots of your codebase or command-line interface does not advance the issue, since the automated testing infrastructure tells us whether the change set still applies correctly.
So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!
See the issue credit guidelines for more information.
Comment #76
alexpottWe should add
To the DrupalPractice section of core/phpcs.xml.dist and fix all the other warnings that that generates. Some like the one in
tests/Drupal/KernelTests/Core/Plugin/Discovery/AnnotatedClassDiscoveryTest.phpare kinda a false positive but that could benew TranslatableMarkup()for cleaner code anyway. Others likecore/lib/Drupal/Core/Action/Plugin/Action/GotoAction.phpprove that we need to use PHPCS to do this :)Comment #77
aldairsoares commentedHi, @alexpott
I got it. My real intention here is to learn how things work in Drupal(I have just started to learn it) and give my little contribution. Credit is good but my main goal is purely to learn more about it.
I took the screenshot just to show that the last patch applied works. But no problem.
I am going to work on your last comment and trying to fix what you pointed.
Thanks.
Comment #78
aldairsoares commentedAs @alexpott asked at #76, I added the code bellow at
core/phpcs.xml.dist
And used composer run phpcs -- -ps core command to find all warnings generated.
The following warnings appeared after that:
So, I worked on fixing them.
PHPCS and the task asked on this issue are solved.
I attached a .txt file with all those warnings.
Needs review.
Comment #79
aldairsoares commentedSending the correct interdiff file. The last one was empty.
Comment #80
aldairsoares commentedI forgot to unassign.
Needs review.
Comment #81
aldairsoares commentedThe last patch had composer in it.
So, I am sending the corrected one.
Needs review.
Comment #83
alexpott@aldairsoares can you add the changes to phpcs.xml.dist so we're permanently checking this. That way we'll not introduce any regressions and fixing everything else will become simpler.
Comment #84
alexpottThis is a test assertion. I think here we should remove the use of t() altogether and hard code the expected string.
This is a test - don't use the trait - use new TranslatableMarkup()
Comment #85
immaculatexavier commentedComment #86
immaculatexavier commentedComment #87
aldairsoares commentedI'm going to work on it.
Comment #88
aldairsoares commented@alexpott,
I made the changes you required at comments #83, #84.1 and #84.2
Needs review.
Comment #90
aldairsoares commentedOne test failed. I'm going to work on it.
Comment #91
aldairsoares commentedI'm sending a new patch with a correction at DisplayTest.php file.
path: core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
I changed the lines #366 and #367 from:
To:
It seems to solve the problem with the last test that failed.
Needs review.
Comment #92
bruno.bicudoI reviewed this one.
It correctly adds the pattern rule
*/Plugin/*to core/phpcs.xml.distAfter running
phpcs -- -psas pointed by @aldairsoares on #78, no more warnings popping out, so i assume tests on all plugins are clean.I also checked for the use of
t()calls andStringTranslationTraiton tests as pointed by @alexpott on #84, and looks like they were all corrected as asked.For the error on patch #88, it's corrected on #91 and the fix looks good for me (just needed the use of
emtags on the text to reflect the correct result for the test, and the additional parentheses was also removed).So far so good, i'm moving it to RTBC as it looks like all the goals were achieved. Thanks!
Comment #93
catchGive this is a 223kb patch I think we might want to schedule it for beta, tentatively putting the week after the beta window in the issue title.
Comment #94
quietone commentedAdd coding standards tag and add the phpcs.xml change to the proposed resolution. Also, this is changing more that field plugins so changing component.
At 124 files changed, 606 insertions, 530 deletions it make sense to schedule this for beta.
Comment #95
alexpottThe
use StringTranslationTrait;should be at the top of the class.This is a test - it shouldn't be using the trait - it can use new TranslatableMarkup...
Comment #96
Ratan Priya commentedComment #97
Ratan Priya commented@alexpott,
I made the changes you required at comments #95
Needs review.
Comment #98
Ratan Priya commentedComment #99
bruno.bicudoI reviewed #97 and the changes reflect what was pointed on #95 by @alexpott
Moving to RTBC again since the changes cover everything which was asked.
Comment #101
quietone commentedRerolling and adding patch for D10.
Comment #102
libbna commentedI have reviewed the above #101 patch for 9.5. And all the
t()functions has been replaced with&this->t().But when I checked for other files there are some files in which
t()is used. LikeDateTime, EntityDisplayRepository, module.api.php,etc. So do we need to make changes in these files also? Though the title says to replace it only in plugins but still confirming it.Not marking it as RTBC because of my doubt.
Comment #103
longwave@Libbna this is only about making this change in plugins, so this issue should only touch code with
/Plugin/in the path - other files will be dealt with in other issues of this issue's parent.Comment #104
libbna commentedThanks @longwave for clarifying my doubt. Then it is good to be marked as RTBC. I don't see any use of
t().Comment #105
quietone commentedThanks for the RTBC!
This needs a reroll now that #3135933: Sort sniffs/rules in phpcs.xml.dist and write test to keep them sorted was committed
Comment #107
quietone commentedComment #108
bbralaLooked through the change in 10.1:
ckeditor is no longer in core but is still in the patch. This needs fixing with a rebase i think.
First started a manual search and did find a few, then i did the automated test. Seems there is a lot to be found. I ran phpcs with phpcs.xml.dist only containing the gloabal snif (as kinda hinted at in #76)
It found a lot of errors. I think the fixer needs a rerun, or the filter (*/Plugin/*) is to broad if it shouldnt also include plugins like:
core/modules/link/src/Plugin/Field/FieldType/LinkItem.ph.Comment #109
bbralaComment #110
quietone commented@bbrala, thanks for the review. However, I do not get the errors you reported when running phpcs on core with the patch applied.
This is a reroll of the 10.1 patch.
Comment #111
quietone commentedAnd a reroll for the 9.5.x patch.
Comment #112
longwaveI also can't reproduce the issues that @bbrala reported.
Reviewed with
git diff --color-words. The only thing that stood out:This comment was only partially removed; as it no longer makes sense we should remove both lines. This was already done in FieldTestItem, this just needs updating to match.
Comment #113
quietone commented@longwave, thanks for the prompt review.
The D10 patch needed a reroll and since only \Drupal\plugin_test\Plugin\plugin_test\mock_block\MockMenuBlockDeriver::getDerivativeDefinitions was affected this patch has the reroll plus the change requested in #112.
Comment #114
quietone commentedAnd repeating the above for 9.5.x.
Comment #115
longwaveThank you. The interdiffs look good and DrupalCI is happy with the PHPCS changes so to me this is RTBC.
Comment #116
alexpottCommitted and pushed 6bbfb05500 to 10.1.x and a4c98c94ee to 10.0.x. Thanks!
Committed ea9a55e and pushed to 9.5.x. Thanks!