Problem/Motivation

\Drupal\language\ConfigurableLanguageManager::getConfigOverrideLanguage() returns NULL if \Drupal\language\Config\LanguageConfigFactoryOverride::setLanguage() or ::setLanguageFromDefault() were not called before. This violates the interface both those classes implement.

Steps to reproduce

(not properly yet, as the fatal occurs in a contrib module doing weird things)

  • Enable more than one language
  • Generate a bunch of content
  • drush search-reindex -y; drush search-index

Detailed backtrace:


Call Stack:
    0.0050     228784   1. {main}() /Users/dawehner/.composer/vendor/drush/drush/drush.php:0
    0.0712     402520   2. drush_main() /Users/dawehner/.composer/vendor/drush/drush/drush.php:12
    0.4679    9603376   3. Drush\Boot\BaseBoot->bootstrap_and_dispatch() /Users/dawehner/.composer/vendor/drush/drush/includes/preflight.inc:68
    0.8950   26225488   4. drush_dispatch(???, ???) /Users/dawehner/.composer/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php:72
    1.3679   30938704   5. call_user_func_array:{/Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:185}(???, ???) /Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:185
    1.3679   30939224   6. drush_command(???) /Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:185
    1.3692   30944016   7. _drush_invoke_hooks(???, ???) /Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:217
    1.3714   30989784   8. call_user_func_array:{/Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:366}(???, ???) /Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:366
    1.3714   30990120   9. drush_core_php_eval(???) /Users/dawehner/.composer/vendor/drush/drush/includes/command.inc:366
    1.3715   30991568  10. eval('drush_module_invoke('search', 'cron');;') /Users/dawehner/.composer/vendor/drush/drush/commands/core/core.drush.inc:1160
    1.3716   30991784  11. drush_module_invoke(???, ???) /Users/dawehner/.composer/vendor/drush/drush/commands/core/core.drush.inc(1160) : eval()'d code:1
    1.3716   30992040  12. Drupal\Core\Extension\ModuleHandler->invoke(???, ???, ???) /Users/dawehner/.composer/vendor/drush/drush/commands/core/drupal/environment.inc:241
    1.3716   30992256  13. call_user_func_array:{/Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php:391}(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php:391
    1.3716   30992512  14. search_cron() /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php:391
    1.4674   34214296  15. Drupal\node\Plugin\Search\NodeSearch->updateIndex() /Users/dawehner/Documents/c3/client/docroot/core/modules/search/search.module:195
    1.4931   37041040  16. Drupal\node\Plugin\Search\NodeSearch->indexNode(???) /Users/dawehner/Documents/c3/client/docroot/core/modules/node/src/Plugin/Search/NodeSearch.php:450
    1.5068   37819384  17. Drupal\Core\Render\Renderer->renderPlain(???) /Users/dawehner/Documents/c3/client/docroot/core/modules/node/src/Plugin/Search/NodeSearch.php:478
    1.5074   37838024  18. Drupal\Core\Render\Renderer->executeInRenderContext(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:152
    1.5074   37838480  19. Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:572
    1.5074   37838592  20. Drupal\Core\Render\Renderer->render(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:151
    1.5074   37839984  21. Drupal\Core\Render\Renderer->doRender(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:195
    1.5097   37888272  22. call_user_func:{/Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:379}(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:379
    1.5097   37888336  23. Drupal\Core\Entity\EntityViewBuilder->build(???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Render/Renderer.php:379
    1.5097   37888896  24. Drupal\Core\Entity\EntityViewBuilder->buildMultiple(???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Entity/EntityViewBuilder.php:203
    1.5658   40563104  25. Drupal\node\NodeViewBuilder->buildComponents(???, ???, ???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Entity/EntityViewBuilder.php:246
    1.5658   40563280  26. Drupal\Core\Entity\EntityViewBuilder->buildComponents(???, ???, ???, ???) /Users/dawehner/Documents/c3/client/docroot/core/modules/node/src/NodeViewBuilder.php:24
    1.6254   44300560  27. Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple(???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Entity/EntityViewBuilder.php:303
    1.6525   45229264  28. Drupal\Core\Field\FormatterBase->view(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php:259
    1.6526   45229320  29. Drupal\address\Plugin\Field\FieldFormatter\AddressDefaultFormatter->viewElements(???, ???) /Users/dawehner/Documents/c3/client/docroot/core/lib/Drupal/Core/Field/FormatterBase.php:80
    1.6526   45232944  30. Drupal\address\Plugin\Field\FieldFormatter\AddressDefaultFormatter->viewElement(???, ???) /Users/dawehner/Documents/c3/client/docroot/modules/contrib/address/src/Plugin/Field/FieldFormatter/AddressDefaultFormatter.php:129
   12.6373   45536224  31. Drupal\address\Plugin\Field\FieldFormatter\AddressDefaultFormatter->getValues(???, ???) /Users/dawehner/Documents/c3/client/docroot/modules/contrib/address/src/Plugin/Field/FieldFormatter/AddressDefaultFormatter.php:150
   12.6495   45851376  32. CommerceGuys\Addressing\Model\Subdivision->hasChildren() /Users/dawehner/Documents/c3/client/docroot/modules/contrib/address/src/Plugin/Field/FieldFormatter/AddressDefaultFormatter.php:281
   12.6495   45851440  33. Doctrine\Common\Collections\AbstractLazyCollection->isEmpty() /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Model/Subdivision.php:241
   12.6495   45851512  34. Doctrine\Common\Collections\AbstractLazyCollection->initialize() /Users/dawehner/Documents/c3/client/docroot/vendor/doctrine/collections/lib/Doctrine/Common/Collections/AbstractLazyCollection.php:85
   12.6495   45851632  35. CommerceGuys\Addressing\Collection\LazySubdivisionCollection->doInitialize() /Users/dawehner/Documents/c3/client/docroot/vendor/doctrine/collections/lib/Doctrine/Common/Collections/AbstractLazyCollection.php:332
   12.6495   45851720  36. CommerceGuys\Addressing\Repository\SubdivisionRepository->getAll(???, ???, ???) /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Collection/LazySubdivisionCollection.php:62
   12.6501   45899656  37. CommerceGuys\Addressing\Repository\SubdivisionRepository->createSubdivisionFromDefinitions(???, ???, ???) /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Repository/SubdivisionRepository.php:94
   12.6502   45900616  38. CommerceGuys\Addressing\Repository\SubdivisionRepository->get(???, ???) /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Repository/SubdivisionRepository.php:228
   12.6502   45901096  39. CommerceGuys\Addressing\Repository\SubdivisionRepository->createSubdivisionFromDefinitions(???, ???, ???) /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Repository/SubdivisionRepository.php:79
   12.6502   45901096  40. CommerceGuys\Addressing\Repository\SubdivisionRepository->translateDefinition(???, ???) /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Repository/SubdivisionRepository.php:214
   12.6502   45901160  41. Drupal\address\Repository\SubdivisionRepository->getDefaultLocale() /Users/dawehner/Documents/c3/client/docroot/vendor/commerceguys/addressing/src/Repository/DefinitionTranslatorTrait.php:21

Proposed resolution

To be determined.

Remaining tasks

To be determined.

User interface changes

None.

API changes

To be determined.

Data model changes

To be determined.

CommentFileSizeAuthor
#36 2684873-36.patch5.12 KBalexpott
#36 2684873-36.test-only.patch1.6 KBalexpott
#31 2684873-30.patch3.52 KBalexpott
#31 28-30-interdiff.txt778 bytesalexpott
#28 2684873-28.patch2.76 KBalexpott
#28 25-28-interdiff.txt684 bytesalexpott
#25 2684873-25.patch2.77 KBalexpott
#19 interdiff.txt1.18 KBXano
#18 drupal_2684873_18.patch3.7 KBXano
#12 2684873-11.patch3.65 KBdawehner
#12 interdiff.txt634 bytesdawehner
#12 interdiff.txt3.39 KBdawehner
#9 2717849-2.patch1.45 KBdawehner
#3 drupal_2684873_2.patch624 bytesXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano created an issue. See original summary.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Active » Needs work

This fixes the symptoms of the problem in a way that makes \Drupal\language\ConfigurableLanguageManager::getConfigOverrideLanguage() act like \Drupal\Core\Language\LanguageManager::getConfigOverrideLanguage() if no override language was set, but it does not fix the root cause.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
FileSize
drupal_2684873_2.patch624 bytes
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs work » Needs review
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Gabor thinks the bug is caused by a module calling getConfigOverrideLanguage() too early, before the language system is initialized.

IRC log:

GaborHojtsy> bojanz: I agree it should conform to its interface
GaborHojtsy> bojanz: not sure though if it returns some language which is just briefly true, is that a good idea?
GaborHojtsy> bojanz: maybe it should rather throw an exception then
GaborHojtsy> bojanz: and then modules can decide how to handle it
GaborHojtsy> bojanz: that is an API change though
GaborHojtsy> bojanz: current callers do not expect an exception
agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

To summarize that: the patch with the fallback will never be committed. If something will change, then an exception will be raised instead of returning NULL. And this change will not land in core before 8.2.0. And no matter, if NULL is returned or an exception will be raised, the caller has to deal with the situation and care for a fallback solution. Right?

@bojanz: that would further mean, that Commerce has to check for emptyness and call getCurrentLanguage() as fallback. Do you agree? Should I create a patch?

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

@agoradesign
This issue is a bug report, which means that the patch is eligible for 8.1.
The fallback is still an option, throwing an exception isn't.
We need to dig deeper to figure out when this happens (just on Drupal install, or always on module install, etc).

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented

As #2 mentions, this patch likely does not fix the root cause, but it shows that the problem starts with the patched method, and not anywhere before that in the call stack. I do not recommend using the patch to temporarily fix the problem, as we haven't yet confirmed it has no side effects.

By looking at the current implementation of the interface, I must conclude that having a config override language is purely optional, and we should either provide a fallback or update the interface documentation to reflect reality, which is that the language is optional and NULL may be returned. That is not necessarily a BC break, as long as this was the originally intended behavior. In that case, this is simply a matter of incorrect documentation.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Issue summary: View changes
FileSize
2717849-2.patch1.45 KB

Here is my patch from the other issue which adds a bit context and documentation. I think on the language manager level its a good idea to always return a language. The language override though is pretty internal, so returning NULL is pretty sane, because it makes the fact that we deal with state more explicit.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs review » Needs work

If @GaborHojtsy has no objections to this approach (and it seemed he didn't on IRC just now), then, apart from the nitpicks below, I think this is good to go!

  1. +++ b/core/modules/language/src/Config/LanguageConfigFactoryOverrideInterface.php
@@ -14,8 +14,9 @@
+   * @return \Drupal\Core\Language\LanguageInterface|NULL

    null must be lowercase when used in documentation. 

  2. +++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -437,7 +437,8 @@ public function setConfigOverrideLanguage(LanguageInterface $language = NULL) {
+    // Note: There might be cases where the language override was not set yet.

    Do we need this notice after we've already updated LanguageConfigFactoryOverrideInterface::getLanguage()'s documentation to include this?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +D8MI, +language-config, +Configuration system, +sprint

Re 2 I think the note is useful but it does not need to include the "Note" prefix :)

Also needs tests.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.39 KB
interdiff.txt634 bytes
2684873-11.patch3.65 KB
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs review » Reviewed & tested by the community

Thanks! One more nitpick, but nothing that blocks this issue, in my opinion.

+++ b/core/modules/language/tests/src/Unit/ConfigurableLanguageManagerTest.php
@@ -0,0 +1,45 @@
+    $language_manager = new ConfigurableLanguageManager(new LanguageDefault(['id' => 'en']), $config_factory, $module_handler->reveal(), $config_override->reveal(), new RequestStack());

I'd put the expected language code in a single variable, so it's slightly clearer that those strings MUST be the same. No blocker, though.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -437,7 +437,8 @@ public function setConfigOverrideLanguage(LanguageInterface $language = NULL) {
+    // There might be cases where the language override was not set yet.
+    return $this->configFactoryOverride->getLanguage() ?: $this->getCurrentLanguage();

I'm not sure about this change... with this change how does a module determine if the override has been set? OTOH the config language will be the current language config at this point. Not sure.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -437,7 +437,8 @@ public function setConfigOverrideLanguage(LanguageInterface $language = NULL) {
+    // There might be cases where the language override was not set yet.
+    return $this->configFactoryOverride->getLanguage() ?: $this->getCurrentLanguage();

I discussed this issue with @dawehner - he pointed about the issues with CLI. In the case of CLI it is not "yet". It never will be set at all. Therefore I think we need to improve this comment.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Well, no matter what we do, we need to document that at some level we return also a NULL ... which is not the case in HEAD. The language though

Maybe it could be interesting to have a DefaultLanguage object which represents that information?

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

Isn't the point of the previously-RTBC patch to never return a NULL? We just need a comment saying why the default is needed (CLI).

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs work » Needs review
FileSize
drupal_2684873_18.patch3.7 KB

What about this?

Personally I don't think this should be documented on the interface, as it's an implementation detail, but this patch tries to accommodate #15. Please let me know what you think.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
FileSize
interdiff.txt1.18 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @xano. I think this will pass the documentation wishes from @alexpott

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review

Thinking about this some more. I'm not sure it is right that LanguageConfigFactoryOverride::getLanguage and ConfigurableLanguageManager:: getConfigOverrideLanguage() ever return something different.

I think rather than make the changes here we need to revisit this code in LangaugeServiceProvider...

    // For monolingual sites, we explicitly set the default language for the
    // language config override service as there is no language negotiation.
    if (!$this->isMultilingual()) {
      $container->getDefinition('language.config_factory_override')
        ->addMethodCall('setLanguageFromDefault', array(new Reference('language.default')));
    }

If we remove the if then override language always defaults to the default before the request language is negotiated.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Or we could even just do an if (!$this->isMultilingual() || PHP_SAPI === 'cli') {

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented

Why is CLI so special? What is the root cause of there not being an override language? I played with https://www.drupal.org/node/add/project-issue/cli_language a bit and don't think I managed to get it to work. I wonder if that's because I made a mistake somewhere or whether the language system by design has less multilingual support in CLI environments.

Regardless of what other changes we make in this issue, ConfigurableLanguageManager still does not properly implements its interface.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Or we could even just do an if (!$this->isMultilingual() || PHP_SAPI === 'cli') {

Do we really have a different container for CLI and NON cli case? I wasn't aware of that, and \Drupal\Core\DrupalKernel::getContainerCacheKey seems to disagree with that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2684873-25.patch2.77 KB

Something like this is probably what we want.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
+++ b/core/modules/language/src/LanguageServiceProvider.php
@@ -54,13 +54,6 @@ public function alter(ContainerBuilder $container) {
-    // For monolingual sites, we explicitly set the default language for the
-    // language config override service as there is no language negotiation.
-    if (!$this->isMultilingual()) {
-      $container->getDefinition('language.config_factory_override')
-        ->addMethodCall('setLanguageFromDefault', array(new Reference('language.default')));
-    }
-

I think this existed from a time when default language was not a container property and we had dependency circles between whats the default language and using config - since the default language is stored in config.

Status: Needs review » Needs work

The last submitted patch, 25: 2684873-25.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
25-28-interdiff.txt684 bytes
2684873-28.patch2.76 KB

Whoops.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

The fix looks great! Triggered a retest.

Should we deprecate setLanguageFromDefault() now that it's no longer used?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
28-30-interdiff.txt778 bytes
2684873-30.patch3.52 KB

@bojanz - yep let's. I think given the constructor change we should target this for 8.3.x

Status: Needs review » Needs work

The last submitted patch, 31: 2684873-30.patch, failed testing.

The last submitted patch, 31: 2684873-30.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Shouldn't we still have some form of test coverage for the bug?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@dawehner yep that shouldn't be a problem.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2684873-36.test-only.patch1.6 KB
2684873-36.patch5.12 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @alexpott!

The last submitted patch, 36: 2684873-36.test-only.patch, failed testing.

  • catch committed f8923c8 on 8.3.x 
    Issue #2684873 by alexpott, dawehner, Xano: ConfigurableLanguageManager...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

Woot, amazing, thanks!

Status: Fixed » Closed (fixed)

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