The method \Drupal\Core\Language\LanguageManager::getLanguages(), depending on the input flags, will add something like this to its return value:
$default->name = $this->t("Site's default language (@lang_name)", array('@lang_name' => $default->name));
$filtered_languages['site_default'] = $default;
The string 'site_default' that it uses as a "language code" should be a constant rather than an explicit string, and the various places all over Core that use this string for various purposes should be updated to use the constant.
Probably it should be defined in \Drupal\Core\Language\LanguageInterface as LANGCODE_SITE_DEFAULT.
Comment | File | Size | Author |
---|---|---|---|
#33 | drupal8-site-default-needs-to-be-language-constant-2328573-33.patch | 12.41 KB | jhodgdon |
#29 | drupal8-site-default-needs-to-be-language-constant-2328573-29.patch | 12.41 KB | jhodgdon |
Comments
Comment #1
jhodgdonThis is probably a good Novice issue.
For convenience, please wait until the issue where this was noticed, #2037979: "Current user's language" views filter label is named very misleading, is committed. Thanks!
Comment #2
el7cosmosComment #3
jhodgdonLooks great! A few notes:
a)
I think in this file it would be best to have a use statement for LanguageInterface so that you don't have to put the whole namespace in this line?
b) Same in core/modules/node/src/Tests/NodeTypeInitialLanguageTest.php
Otherwise, looks great! I also verified that you have found and fixed all the instances of site_default and replaced them with this new constant.
And sorry for the delay in reviewing -- I was away for the last 10 days.
Comment #4
el7cosmosAlso in
/core/modules/views/src/Plugin/views/PluginBase.php
and/core/modules/views/views.api.php
there is string like:Does this need to be change?
Comment #5
el7cosmosComment #6
jhodgdonThanks! In the future, when you add a new patch to an issue, please provide an interdiff file as well.
And yes, those strings in PluginBase and views.api.php would be good to change too, thanks!
Comment #7
el7cosmosSorry for the missing interdiff. Here is the interdiff for previous two patches.
Comment #8
el7cosmosReorder the files, hope doesn't mess it.
Comment #10
el7cosmosComment #11
jhodgdonThanks! Looks great to me.
Comment #12
alexpottI think we should be doing
$id = '***LANGUAGE_site_default***';
since the is not the same thing as the language interface constant. See below.I'm not sure about these changes - this is a placeholder in views and will be written the yaml files I think the hardcoding is correct.
Comment #13
jhodgdonI'm not sure about #12. We are doing the same thing of '***LANGUAGE_' . (a constant from somewhere else) . '***' in the rest of those same methods in PluginBase, so do you think we should be changing those too?
The IDs in getDefinedLanguageTypesInfo() are constants from LanguageInterface as well. By this logic, we should hard-code these to be new Views constants as well. I don't think this is a good idea?
Comment #14
jhodgdon@alexpott: So do you think we should change all of the ***LANGUAGE_ things so that they do not use the constants defined in LanguageInterface? If not, I think we should not change this one either.
Comment #15
alexpottIt just feels super weird to be adding a constant to a fixed string - that just results in a constant. The difference with the language types is that there is hook to create new ones. But we have
in head already :)
And in the patch we have this... making the example implementation icky and inconsistent.
Not sure how to proceed actually.
Comment #16
jhodgdonI talked to alexpott in IRC and what we'd like to have happen for the '***LANGUAGE_site_default***' constant is:
- Add it as a constant in the views PluginBase class, with an @see to the corresponding (new) constant in LanguageInterface
- Use this constant in the two methods in PluginBase, instead of
$id = '***LANGUAGE_' . $id . '***';
Comment #17
YesCT CreditAttribution: YesCT commentedrerolling now. then will make the new const.
Comment #18
YesCT CreditAttribution: YesCT commentedrerolling for
#2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete()
#2338011: Cleanup of locale tests
next to do the change in the view constant.
Comment #19
el7cosmos@YesCT thanks for rerolling the patch.
@jhodgdon @alexpott what would be the name of the constant?
Comment #20
YesCT CreditAttribution: YesCT commentedtricky to name things, but VIEWS_QUERY_LANGUAGE_SITE_DEFAULT was suggested in irc. I'll do it now. patch coming.
Comment #21
YesCT CreditAttribution: YesCT commentedlet's try this.
Comment #22
jhodgdonSeems like the wrong patch was uploaded in #21, or it has a lot of extra stuff in it? And the interdiff doesn't include a definition of the new constant... so something is wrong?
Comment #23
el7cosmosHow about this?
Comment #24
jhodgdonThe patch looks good to me, thanks! However, it doesn't quite apply; needs a reroll (context change in LanguageConfigurationElementTest). Which I've done. Didn't make any other changes in the patch.
So, I reviewed the patch carefully, and also applied the patch and verified that all instances of site_default in the code base were taken care of, with and without the Views prefix. I think this is ready to go.
Comment #26
jhodgdonOh, that was the #23 patch that failed due to needing reroll... still waiting on #24 (reroll) tests to complete.
Comment #27
webchickThis looks straight-forward to me, but seems like alexpott had some opinions about it.
Comment #29
jhodgdonNew reroll. Apparently some of language.module moved into core/modules/language/src/Element/LanguageConfiguration.php so the corresponding part of the patch needed to move there too. Again validated via grep that this patch takes care of all of the 'site_default' strings...
Comment #33
jhodgdonApparently I cannot spell. When I moved that chunk into the LanguageConfiguration class... typo. whoops.
Comment #34
holist CreditAttribution: holist commentedI will look into this.
Comment #35
holist CreditAttribution: holist commentedNothing wrong with this to my eye.
Comment #36
alexpottCommitted 5a52005 and pushed to 8.0.x. Thanks!