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.

Comments

Xano created an issue. See original summary.

Xano’s picture

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’s picture

Status: Needs work » Needs review
bojanz’s picture

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

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

@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

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

Issue summary: View changes
FileSize
1.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

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

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

Also needs tests.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.39 KB
634 bytes
3.65 KB
Xano’s picture

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

+++ 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

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

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

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

Status: Needs work » Needs review
FileSize
3.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

FileSize
1.18 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

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

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

Xano’s picture

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

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

Something like this is probably what we want.

alexpott’s picture

+++ 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

Status: Needs work » Needs review
FileSize
684 bytes
2.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

The fix looks great! Triggered a retest.

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

alexpott’s picture

@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

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

alexpott’s picture

@dawehner yep that shouldn't be a problem.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
5.12 KB
alexpott’s picture

dawehner’s picture

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

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, amazing, thanks!

Status: Fixed » Closed (fixed)

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