Problem/Motivation

This is a componentized piece broken out of #2201437: [META-1] Config overrides and language. The proposal there is to move language overrides to be a regular config override and therefore even those would involve the events system. For such a core operation as a configuration override, especially when invoked on all sites using configuration, it is important that this goes fast. Part of the proposal in #2201437: [META-1] Config overrides and language was to move from an event subscriber system to override services, which are compiled to the Drupal kernel. Therefore if no modules are enabled which provide override services, no time is wasted calling for overrides.

Proposed resolution

  1. Instead of using events for overrides, use services tagged with config.factory.override. The relative priority of these services defines the priority of the overrides themselves.
  2. Add a ConfigFactoryOverrideInterface to define these services, so we have a standard way to retrieve overrides from them.
  3. Modify the config factory to be able to take these overrides and use them when needed.
  4. Modify existing override event listeners to now be services tagged as config factory overrides (only affects tests due to not having any other implementation of module overrides).

This does not actually change anything regards to language handling per say. Moving that to an override service instead of a baked-in configuration solution would require much wider changes, eg. moving language responsibilities from Config an ConfigFactory to LanguageManagers, etc. That would be most of the patch from #2201437: [META-1] Config overrides and language. Instead this issue is focusing on changing the module override system for now and we can move the language overrides to this system in a followup.

Remaining tasks

  1. Review.
  2. Argue.
  3. Redo/update.
  4. Review.
  5. Commit

User interface changes

None.

API changes

  • The 'config.module.overrides' event is removed.
  • ConfigFactoryOverrideInterface added to be implemented by services providing overrides.
  • Instead of services tagged as event_subscriber (implementing EventSubscriberInterface), now overrides are provided by services tagged as config.factory.override (implementing ConfigFactoryOverrideInterface)
Files: 
CommentFileSizeAuthor
#15 interdiff.txt1.86 KBWim Leers
#15 override-service-config-15.patch14.46 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,394 pass(es). View

Comments

Gábor Hojtsy’s picture

xjm’s picture

Priority: Major » Critical
Issue tags: +beta blocker
Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

FileSize
14.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,315 pass(es). View
alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
93.54 KB

In itself this is just replacing events with a compiler pass and tagged services. It does just in a very very minor performance boost - but I would argue that configuration read is one part of the system when all performance optimisations are worthwhile.


A XHprof comparision of loadModuleOverrides during a standard profile install with and without the patch. The gain here is entirely due to not using the eventdispatcher.

Furthermore when we expand ConfigFactoryOverrideInterface with additional methods for config installation and import it will anything that overrides configuration to react to the configuration system workflow in a nice and organised fashion.

Using compiler passes to implement additional functionality like this is not unique to ConfigFactory - this pattern is copied from \Drupal\Core\Theme\ThemeNegotiator

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I should not have rtbc'd this being that it is a rework of my work on #2201437: [META-1] Config overrides and language - reviews please.

catch’s picture

Status: Needs review » Needs work

Quick review, agree with the overall direction and 7ms would be a decent performance improvement if our overall performance hadn't regressed so much.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverridePass.php
    @@ -0,0 +1,33 @@
    +  public function process(ContainerBuilder $container) {
    

    Could we also sort them here? Looks like we could do that after the foreach and save it on runtime.

  2. +++ b/core/modules/config/tests/config_override/lib/Drupal/config_override/ConfigOverriderLowPriority.php
    @@ -0,0 +1,38 @@
    +            // This override should apply because it is not overridden by the
    +            // higher priority listener.
    +            'name' => 'Should not apply because of higher priority listener',
    +            'slogan' => 'Yay for overrides!',
    +          )
    

    The comment says should, the name says should not.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
14.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,366 pass(es). View
6.33 KB

@catch: here is an updated patch addressing your concerns:

1. Sorts the services on compile. This makes the addOverrides() somewhat of a lame duck, it can now only add services at the end of the priority list (lower priority compared to existing ones). If we say overrides should only be added from the compiled list, then this is equally capable.

2. Moved the comment, it was related to the second line (slogan). The first (name) does not apply exactly due to this being a low priority override and the higher one having a value for it, this is tested in the tests :)

catch’s picture

That looks better to me, thanks :)

Gábor Hojtsy’s picture

Any other concerns or looks good to go? @alexpott what do you think about the new priority ordering solution?

alexpott’s picture

@Gábor Hojtsy I think it fine to only allow override objects to be ordered properly during the compiler pass - and any that are added dynamically have to go last.

xjm’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -392,4 +404,11 @@ static function getSubscribedEvents() {
+  public function addOverride(ConfigFactoryOverrideInterface $config_factory_override) {
+    $this->configFactoryOverrides[] = $config_factory_override;

Do we need to worry about duplicates being added?

xjm’s picture

+++ b/core/modules/config/tests/config_override/lib/Drupal/config_override/ConfigOverrider.php
@@ -0,0 +1,37 @@
+<?Php

+++ b/core/modules/config/tests/config_override/lib/Drupal/config_override/ConfigOverriderLowPriority.php
@@ -0,0 +1,38 @@
+<?Php

Also, this is like... the ultimate nitpick... but these <?php tags shouldn't be capitalized.

Gábor Hojtsy’s picture

FileSize
14.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,289 pass(es). View

@xjm: as for duplicates I don't think we need to worry, there are no side effects other than slowing down the process a bit. If override services A, B, B, C and B are added in this order, then the first instance of B will already apply all of B's overrides. The array merges will find the second instance of B and then the third instance of B 'useless' because all items attempted to be overriden from there are already applied. C will work the same way as if there was only one B. So duplicates would only cause some useless processing.

Also there is not a lot of chance of duplicates happening unless different service names are used to register the same service class or addOverrides() is called runtime outside of the compiled code to add overrides with an override service that was already added (not likely :D).

Edited the patch to have php instead of Php. No other changes so no interdiff. Amusingly this was a pre-existing problem in the existing files we remove :D

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
14.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,394 pass(es). View
1.86 KB

I can confirm #14, because NestedArray::mergeDeepArray() is also used to ensure JavaScript settings are merged in an idempotent manner (see drupal_merge_js_settings()) and https://drupal.org/node/1911578.

Test coverage makes sense. Code makes sense. I could only find a few nitpicks, which I fixed in this reroll. That shouldn't prevent an RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Yay, now we can continue breaking out #2201437: [META-1] Config overrides and language :) Thanks!

gremy’s picture

The Configuration override system (https://drupal.org/node/1928898) documentation page needs to be updated to match current implementation.

Status: Fixed » Closed (fixed)

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