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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue tags: +Novice

This 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!

el7cosmos’s picture

Status: Active » Needs review
FileSize
8.82 KB
jhodgdon’s picture

Status: Needs review » Needs work

Looks great! A few notes:

a)

+++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
@@ -76,7 +76,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
       // Check whether we have any custom setting.
       foreach ($bundles[$entity_type_id] as $bundle => $bundle_info) {
         $conf = language_get_default_configuration($entity_type_id, $bundle);
-        if (!empty($conf['language_show']) || $conf['langcode'] != 'site_default') {
+        if (!empty($conf['language_show']) || $conf['langcode'] != \Drupal\Core\Language\LanguageInterface::LANGCODE_SITE_DEFAULT) {

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.

el7cosmos’s picture

Also in /core/modules/views/src/Plugin/views/PluginBase.php and /core/modules/views/views.api.php there is string like:

***LANGUAGE_site_default***

Does this need to be change?

el7cosmos’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! 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!

el7cosmos’s picture

Sorry for the missing interdiff. Here is the interdiff for previous two patches.

el7cosmos’s picture

Reorder the files, hope doesn't mess it.

Status: Needs review » Needs work
el7cosmos’s picture

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks great to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -471,12 +471,12 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL) {
    -      if ($id == 'site_default') {
    +      if ($id == LanguageInterface::LANGCODE_SITE_DEFAULT) {
             $id = '***LANGUAGE_' . $id . '***';
           }
    

    I think we should be doing $id = '***LANGUAGE_site_default***'; since the is not the same thing as the language interface constant. See below.

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -517,7 +517,7 @@ public static function queryLanguageSubstitutions() {
    -    $changes['***LANGUAGE_site_default***'] = $default;
    +    $changes['***LANGUAGE_' . LanguageInterface::LANGCODE_SITE_DEFAULT . '***'] = $default;
    
    +++ b/core/modules/views/views.api.php
    @@ -578,7 +578,7 @@ function hook_views_query_substitutions(ViewExecutable $view) {
    -    '***LANGUAGE_site_default***' => \Drupal::languageManager()->getDefaultLanguage()->id,
    +    '***LANGUAGE_' . LanguageInterface::LANGCODE_SITE_DEFAULT . '***' => \Drupal::languageManager()->getDefaultLanguage()->id,
    

    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.

jhodgdon’s picture

Status: Needs work » Needs review

I'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?

// in listLanguages:
      $types = $manager->getDefinedLanguageTypesInfo();
      foreach ($types as $id => $type) {
           $id = '***LANGUAGE_' . $id . '***';
          $list[$id] = t('Language selected for !type', array('!type' => $type['name']));
        }
// in queryLanguageSubstitutions:
    $types = $manager->getDefinedLanguageTypesInfo();
    foreach ($types as $id => $type) {
      if (isset($type['name'])) {
        $changes['***LANGUAGE_' . $id . '***'] = $manager->getCurrentLanguage($id)->id;

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?

jhodgdon’s picture

@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.

alexpott’s picture

It 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

      if ($id == 'site_default') {
        $id = '***LANGUAGE_' . $id . '***';
      }

in head already :)

+++ b/core/modules/views/views.api.php
@@ -578,7 +578,7 @@ function hook_views_query_substitutions(ViewExecutable $view) {
     '***LANGUAGE_language_content***' => \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->id,
-    '***LANGUAGE_site_default***' => \Drupal::languageManager()->getDefaultLanguage()->id,
+    '***LANGUAGE_' . LanguageInterface::LANGCODE_SITE_DEFAULT . '***' => \Drupal::languageManager()->getDefaultLanguage()->id,

And in the patch we have this... making the example implementation icky and inconsistent.

Not sure how to proceed actually.

jhodgdon’s picture

I 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 . '***';

YesCT’s picture

Status: Needs review » Needs work

rerolling now. then will make the new const.

YesCT’s picture

el7cosmos’s picture

@YesCT thanks for rerolling the patch.

@jhodgdon @alexpott what would be the name of the constant?

YesCT’s picture

tricky to name things, but VIEWS_QUERY_LANGUAGE_SITE_DEFAULT was suggested in irc. I'll do it now. patch coming.

YesCT’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Seems 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?

el7cosmos’s picture

How about this?

jhodgdon’s picture

The 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.

jhodgdon’s picture

Oh, that was the #23 patch that failed due to needing reroll... still waiting on #24 (reroll) tests to complete.

webchick’s picture

Assigned: Unassigned » alexpott

This looks straight-forward to me, but seems like alexpott had some opinions about it.

Status: Reviewed & tested by the community » Needs work
jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.41 KB

New 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...

Status: Reviewed & tested by the community » Needs work

Status: Needs work » Needs review

Status: Needs review » Needs work
jhodgdon’s picture

Status: Needs work » Needs review
FileSize
12.41 KB

Apparently I cannot spell. When I moved that chunk into the LanguageConfiguration class... typo. whoops.

holist’s picture

Issue tags: +Amsterdam2014

I will look into this.

holist’s picture

Status: Needs review » Reviewed & tested by the community

Nothing wrong with this to my eye.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5a52005 and pushed to 8.0.x. Thanks!

  • alexpott committed 5a52005 on 8.0.x
    Issue #2328573 by el7cosmos, jhodgdon, YesCT: 'site_default' needs to be...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.