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.
Comment | File | Size | Author |
---|---|---|---|
#36 | 2684873-36.patch | 5.12 KB | alexpott |
#36 | 2684873-36.test-only.patch | 1.6 KB | alexpott |
#31 | 2684873-30.patch | 3.52 KB | alexpott |
#31 | 28-30-interdiff.txt | 778 bytes | alexpott |
#28 | 2684873-28.patch | 2.76 KB | alexpott |
Comments
Comment #2
XanoThis 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.Comment #3
XanoComment #4
XanoComment #5
bojanz CreditAttribution: bojanz at Centarro commentedGabor thinks the bug is caused by a module calling getConfigOverrideLanguage() too early, before the language system is initialized.
IRC log:
Comment #6
agoradesign CreditAttribution: agoradesign commentedTo 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?
Comment #7
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).
Comment #8
XanoAs #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.Comment #9
dawehnerHere 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.
Comment #10
XanoIf @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!
null
must be lowercase when used in documentation.Do we need this notice after we've already updated
LanguageConfigFactoryOverrideInterface::getLanguage()
's documentation to include this?Comment #11
Gábor HojtsyRe 2 I think the note is useful but it does not need to include the "Note" prefix :)
Also needs tests.
Comment #12
dawehnerComment #13
XanoThanks! One more nitpick, but nothing that blocks this issue, in my opinion.
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.
Comment #14
alexpottI'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.
Comment #15
alexpottI 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.
Comment #16
dawehnerWell, 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?Comment #17
bojanz CreditAttribution: bojanz at Centarro commentedIsn'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).
Comment #18
XanoWhat 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.
Comment #19
XanoComment #20
dawehnerThank you @xano. I think this will pass the documentation wishes from @alexpott
Comment #21
alexpottThinking about this some more. I'm not sure it is right that
LanguageConfigFactoryOverride::getLanguage
andConfigurableLanguageManager:: getConfigOverrideLanguage()
ever return something different.I think rather than make the changes here we need to revisit this code in LangaugeServiceProvider...
If we remove the if then override language always defaults to the default before the request language is negotiated.
Comment #22
alexpottOr we could even just do an
if (!$this->isMultilingual() || PHP_SAPI === 'cli') {
Comment #23
XanoWhy 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.Comment #24
dawehnerDo 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.Comment #25
alexpottSomething like this is probably what we want.
Comment #26
alexpottI 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.
Comment #28
alexpottWhoops.
Comment #30
bojanz CreditAttribution: bojanz at Centarro commentedThe fix looks great! Triggered a retest.
Should we deprecate setLanguageFromDefault() now that it's no longer used?
Comment #31
alexpott@bojanz - yep let's. I think given the constructor change we should target this for 8.3.x
Comment #34
dawehnerShouldn't we still have some form of test coverage for the bug?
Comment #35
alexpott@dawehner yep that shouldn't be a problem.
Comment #36
alexpottComment #37
alexpottComment #38
dawehnerThank you @alexpott!
Comment #41
catchCommitted/pushed to 8.3.x, thanks!
Comment #42
Gábor HojtsyWoot, amazing, thanks!