Use services instead of event subscribers for config module overrides [#2215503]

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

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.
Add a ConfigFactoryOverrideInterface to define these services, so we have a standard way to retrieve overrides from them.
Modify the config factory to be able to take these overrides and use them when needed.
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

Review.
Argue.
Redo/update.
Review.
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)

Comment	File	Size	Author
#15	interdiff.txt	1.86 KB	Wim Leers
#15	override-service-config-15.patch	14.46 KB	Wim Leers
#15
#14	override-service-config-14.patch	14.37 KB	Gábor Hojtsy
#14
#8	interdiff.txt	6.33 KB	Gábor Hojtsy
#8	override-service-config-8.patch	14.37 KB	Gábor Hojtsy
#8
#5	loadOverrides.png	93.54 KB	alexpott
#4	override-service-config.patch	14.92 KB	Gábor Hojtsy
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 11 March 2014 at 14:54

File	Size
override-service-config.patch	14.92 KB

Comment #2

xjm

she/her

English

CreditAttribution: xjm commented 11 March 2014 at 14:58

Priority:	Major	» Critical
Issue tags:		+beta blocker

1 file was hidden/shown/deleted

File	Size
override-service-config.patch	14.92 KB

Comment #3

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 11 March 2014 at 15:03

Issue summary:

View changes

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 11 March 2014 at 15:06

File	Size
override-service-config.patch	14.92 KB

Comment #5

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 12 March 2014 at 10:04

Status:

Needs review

» Reviewed & tested by the community

File	Size
loadOverrides.png	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

Comment #6

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 12 March 2014 at 13:50

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.

Comment #7

catch

he/him

English

CreditAttribution: catch commented 12 March 2014 at 14:41

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.

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

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

Comment #8

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 12 March 2014 at 16:47

Status:

Needs work

» Needs review

File	Size
override-service-config-8.patch	14.37 KB

interdiff.txt	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 :)

Comment #9

catch

he/him

English

CreditAttribution: catch commented 12 March 2014 at 20:36

That looks better to me, thanks :)

Comment #10

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 13 March 2014 at 09:56

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

Comment #11

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 13 March 2014 at 10:21

@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.

Comment #12

xjm

she/her

English

CreditAttribution: xjm commented 13 March 2014 at 10:21

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

Comment #13

xjm

she/her

English

CreditAttribution: xjm commented 13 March 2014 at 10:23

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

Comment #14

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 13 March 2014 at 10:42

File	Size
override-service-config-14.patch	14.37 KB

@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

Comment #15

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 13 March 2014 at 19:33

Status:

Needs review

» Reviewed & tested by the community

File	Size
override-service-config-15.patch	14.46 KB

interdiff.txt	1.86 KB

5 files were hidden/shown/deleted

File	Size
override-service-config.patch	14.92 KB

loadOverrides.png	93.54 KB
override-service-config-8.patch	14.37 KB

interdiff.txt	6.33 KB
override-service-config-14.patch	14.37 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.