Problem/Motivation
Language module alters definition of 'language_manager' service from core.services.yml (see LanguageServiceProvider::alter()):
<?php
public function alter(ContainerBuilder $container) {
$definition = $container->getDefinition('language_manager');
$definition->setClass('Drupal\language\ConfigurableLanguageManager')
->addArgument(new Reference('config.factory'))
->addArgument(new Reference('module_handler'))
->addArgument(new Reference('language.config_factory_override'))
->addArgument(new Reference('request_stack'));
if ($default_language_values = $this->getDefaultLanguageValues()) {
$container->setParameter('language.default_values', $default_language_values);
}
}
?>This way of altering the service makes it hard for non-core modules to decorate the service, since the decorator cannot know the class of decorated service.
Proposed resolution
Instead of altering language manager service, language module could declare another public service, which will decorate language manager service.
Remaining tasks
- Create the new service (language.language_manager?) in language.services.yml
- Remove LanguageServiceProvider::alter() method
- Update the code of language module to use the new service
- Update the tests
User interface changes
No UI changes
API changes
New service language.language_manager, existing only when language module is enabled
Data model changes
No data model changes.
Release notes snippet
Language module exposes its own, extended language_manager service as language.language_manager
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | patchOnD9.png | 85.86 KB | admin@azhark.com |
| #4 | 3032067-4.patch | 2.69 KB | valthebald |
| #2 | 3030681.patch | 5.4 KB | valthebald |
Issue fork drupal-3032067
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
valthebaldFirst take
Comment #4
valthebaldComment #9
siddhant.bhosale commentedComment #10
Yavor Petrov commentedI was strugguling with this for couple of hours and wasn't able to overwrite/decorate the LanguageServiceProvider to load my own LanguageManager in which I overwrite the getLanguages method. This is needed to filter some languages depending on the role and dynamic custom field of the user. So I created a new module starting with z (e.g. z_custom_overwritemodule) because the order of module execution is alphabetical. This way my custom ServiceProvider loads after the language's module one. Not the best solution but at least it works :)
Comment #11
valthebaldPatch safely applies, moving to Needs review. I would argue that since this is kind of refactoring, no additional tests needed?
Comment #16
akalam commentedWhen trying to apply the patch on D8.9 project I got the following fatal error:
After checking other branches (9.0.x, 9.1.x, 9.2.x and 9.3.x) I see the ConfigurableLanguageManager receives the default language in its constructor, so I think there is a missing argument in the service declaration.
Added a MR against 9.3.x branch with a new patch fix this issue. I can confirm the patch applies and is working in D8.9 sites.
Comment #17
admin@azhark.com commented@akalam I checked on 9.3.x-dev, the 3032067-4.patch is working fine.
If possible add the changed patch which you created for D8.9
Comment #21
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Tagging for tests to show the new service functionality
This will need an upgrade path for the backwards compatibility since it's changing existing services. Instead of changing them may need to pass in the new service and have a trigger_error.
Needs a change record to announce the new service and changes to existing one.
Thanks!
Comment #23
acbramley commentedThis has been triaged as part of bug smash. Along with #21 this also needs a reroll and retarget of the MR to 11.x