Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In InstallerTestBase::setUpLanguage we must not translate the 'Save and continue' message, because in that state, the language is always english.
Proposed resolution
Remove the use of $this->translations
on the 'Save and continue' button text.
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff-18-27.txt | 798 bytes | jungle |
#27 | 2691389-27.patch | 820 bytes | jungle |
Comments
Comment #2
chr.fritschHere is the patch
Comment #3
chr.fritschComment #11
Kristen PolNeeds a reroll. See
core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
.Comment #12
mrinalini9 CreditAttribution: mrinalini9 at Material for Drupal India Association commentedComment #13
mrinalini9 CreditAttribution: mrinalini9 at Material for Drupal India Association commentedRerolled patch #2 for 9.1.x branch, please review.
Comment #14
Kristen PolComment #15
Kristen PolI'm trying to understand if this change makes sense to me. Looking at the existing code in
InstallerTestBase.php
, I see'Save and continue'
used in:Other than the above and other tests, I see
'Save and continue'
in:where
SelectLanguageForm.php
is intentionally not using translation as noted in the comments:There are many classes that extend
InstallerTestBase
. For example,core/tests/Drupal/FunctionalTests/Installer/InstallerTranslationMultipleLanguageTest.php
where the parent method is called after adding the translation files:This looks like, while the translation files are written, they aren't yet loaded so the translations would not be available yet.
So, if I'm understanding correctly, the reason the code works before patching is because there is no translation yet available so English is just passed through as is. In that, the change makes sense to me. Given this and the following I'm marking RTBC:
1) Patch applies
2) Tests pass
3) Change is straightforward and addresses the problem noted in the issue summary
4) No manual testing should be need as it should be covered by automated testing
Though I'm wondering if it could use a comment so that it's clear why translation isn't used (similar to the comments for
SelectLanguageForm.php
).Whether it's worth committing to core, that is update to the core maintainers. :)
Comment #16
Kristen PolPulling this one back after thinking about it more. I think it needs a comment. I would suggest something like:
Comment #17
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #18
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have addressed comment #16.
Comment #19
Kristen PolThanks for the update.
1) Interdiff looks good.
2) Tests still pass.
3) Based on this and #15, marking RTBC.
Comment #20
jungleThe translations property should be removed. no usages in child classes., edited, I was wrong.The comment is unnecessary to me, but happy to add it back.
Comment #21
jungleAddressing #20. Not sure what I am doing is right, let's waiting for the testing run finishes.
Comment #23
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented$translations is used by some child class like ConfigTranslationInstallTest who extending InstallerTestBase class so not removing this and just removing $this->translations from InstallerTestBase class. Kindly review a new patch
Comment #24
jungleThinking again, I would say that this is a won't fix. The original code is ok to me. It does not call
t()
nornew TranslatableMarkup()
,Dont translate
in the title does not make sense.Comment #26
quietone CreditAttribution: quietone commentedThese are all out of scope. The IS specifically says this is for \Drupal\FunctionalTests\Installer\InstallerTestBase::setUpLanguage() only.
Now the question is the 'select language' step of the install process only in English. I wasn't sure so I tested manually. Yes, it is always in English. The 'Save and continue' text is always English no matter what non-English language I selected for the installation. Based on that I would say the patch in #18 is on the right track. It just needs an improved comment.
This would be better if we included where this is hard coded (save the next dev some time). Something like this "The 'Select Language' step is always English.
@See \Drupal\Core\Installer\Form\SelectLanguageForm."
Comment #27
jungleThanks @quietone for the review.
Adjusted per @quietone's suggestion based on #18. But I still do not think the change/improvement here is necessary as I commented in #24.
Comment #28
Gábor HojtsyThis makes sense. Its a minor fix at best though :) Thanks for looking at this.
Comment #30
catchCommitted 9b4c2f0 and pushed to 9.1.x. Thanks!