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
We have a pattern where we get a Language from languageManager() and then later call getLanguageConfigOverride(). But getLanguageConfigOverride() is only for ConfigurableLanguage (managers).
for example in
ConfigLanguageOverrideWebTest::testSiteNameTranslation()
\Drupal::languageManager()
->getLanguageConfigOverride($langcode, 'system.site')
->set('name', 'XX site name')
->save();
(note maybe also added in #1879930: Language selectors are not showing localized to the page language depending on how that goes in.)
Proposed resolution
use a configurable language manager when we need config things.
Remaining tasks
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| Create a patch | Instructions | ||
| Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
No.
API changes
No.
| Comment | File | Size | Author |
|---|---|---|---|
| #49 | interdiff_48-49.txt | 5.23 KB | pooja saraah |
| #49 | 2355665-49.patch | 27.04 KB | pooja saraah |
| #48 | reroll_diff_35-48.txt | 33.61 KB | sahil.goyal |
| #48 | 2355665-48.patch | 27.09 KB | sahil.goyal |
| #35 | use_get_a-2355665-35.patch | 14.78 KB | Manuel Garcia |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
cheops90 CreditAttribution: cheops90 commentedComment #3
cheops90 CreditAttribution: cheops90 commentedComment #4
cheops90 CreditAttribution: cheops90 commentedComment #5
XanoIs this done just for type hinting? If so, try doing the following instead. Most IDEs can parse this.
Comment #6
cheops90 CreditAttribution: cheops90 commentedWorking on it: Change the type hinting.
Comment #7
YesCT CreditAttribution: YesCT commentedJust noting that all instances of this are in tests.
Comment #8
cheops90 CreditAttribution: cheops90 commentedComment #9
cheops90 CreditAttribution: cheops90 commentedSorry the comment id was wrong.
Comment #10
cheops90 CreditAttribution: cheops90 commentedComment #11
YesCT CreditAttribution: YesCT commentedIt is common, and helps reviewers, if when posting a new patch on an issue (that already has a patch) to include an interdiff. For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff
Comment #12
YesCT CreditAttribution: YesCT commentedchanges like this seem out of scope for this issue...
I think #2351251: Replace extend of deprecated DrupalUnitTestBase with KernelTestBase in Config covers that.
is languageManager already a property we can set?
if not, we should probably declare it.
Comment #13
cheops90 CreditAttribution: cheops90 commentedPatch from #7 and #9 are exactly the same - only the name is different.
Comment #14
XanoThat's better! Almost there :)
This is useless, because you are assigning the language manager to a local variable without using it. The type hint should be done where you are calling methods on the object.
Are you using an IDE like PhpStorm?
Comment #15
YesCT CreditAttribution: YesCT commentedThanks!
Dont worry if the patch number gets off by 1 or so from the comment number. that happens sometimes. :)
looks like the out of scope changes are reverted. thanks.
why like this instead of $this->languageManager like the others?
Comment #16
cheops90 CreditAttribution: cheops90 commented@Xano: You are right this is useless, I removed it. Yes i use PhpStorm.
@YesCT: I used $this->container->get('language_manager') because we need only once languageManager in this class, but for the future improvements i think it is better to have a property, so i changed this too.
Thanks.
Comment #19
XanoCool! Then if you write something like the following code, then right click (in PhpStorm) on
getLanguageConfigOverrideStorageand select Go to > Declaration, PhpStorm will show you the method's original declaration in the interface, all thanks to the@vartype hint. The other way around works too: if you don't type hint, PhpStorm will think you are calling a non-existing method and will warn you about it.As you can see, we should have type hinted on
ConfigurableLanguageManagerInterfaceinstead ofConfigurableLanguageManager(always type hint on the interface if there is one). Sorry that I put you on the wrong track!Comment #21
cheops90 CreditAttribution: cheops90 commentedChanges ConfigurableLanguageManager with ConfigurableLanguageManagerInterface.
Comment #22
cheops90 CreditAttribution: cheops90 commentedComment #27
YesCT CreditAttribution: YesCT commentedmaybe we cannot reuse the one in the property and need to
$this->languageManager = $this->container->get('language_manager');
again. (with a comment explaining why)
(or not use a property, and just local typehint it.)
Comment #28
cheops90 CreditAttribution: cheops90 commentedComment #29
cheops90 CreditAttribution: cheops90 commentedComment #30
YesCT CreditAttribution: YesCT commentednice. it passes.
I feel like we need someone to *think* about the approach of having the properties.
Comment #33
quietone CreditAttribution: quietone as a volunteer commentedPatch no longer applies cleanly.
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commentedRerolling...
Comment #35
Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commentedManually fixed conflicts in:
Comment #36
Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commentedComment #47
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This will need to be rerolled for 10.1.x
Comment #48
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedrerolling the patch against version 10.1.x and attaching the reroll_diff and few more changes are done also to get rid of PHP coding standard issues.
Comment #49
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed failed commands on #48
Attached patch against Drupal 10.1.x
Comment #50
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commented