Problem/Motivation

The Config object depends on the Event Dispatcher which depends on the Kernel which depends on PHP storage which by default depends on finding the public files directory which is retrieved using the Config object. This is a circular dependency.

The simple case of using settings.php to override very early configuration values is not working because , again, we need the whole Kernel to be up.

Yet, the write events are not reliable because config import and module install and whoever else wants to do so can bypass Config by using the storage layer instead. There is, in fact, no way, to react to the import or the install of new config. This has bitten me when I was trying to write my metadata patch and while #1846454-22: Add Entity query to Config entities is a good idea it is not doable at this moment.

Proposed resolution

  1. Move the file path into settings.php as you need to copy the config directory anyways before you can change the file path so changing it is not supported from the UI anyways (catch pointed this out).
  2. Move the $conf override to something non-kernel dependent
  3. Remove the write events because they are unusuable.

Remaining tasks

Figure something out for writes which, again, is not kernel dependent.

User interface changes

None.

API changes

We shall see.

Comments

Gábor Hojtsy’s picture

I believe the reason we were forced to not use hooks is exactly because we'll need config before the hook system is initialised.

Jose Reyero’s picture

@chx,
The way you put it, it sounds like it is broken and it is not. It works.
(Btw the language override subscriber is initialized on language initialization so I don't understand that one)

Whether using event dispatcher is a good idea or not ok, maybe there's a better option, but certainly not hooks, and the reason is clearly explained by @Gábor Hojtsy in #1

I we want to explore better options, maybe plugins is the answer -they just weren't ready by the time we added the feature- so maybe it's time to rethink it.

beejeebus’s picture

Issue tags: +Configuration system

tagging.

catch’s picture

The bug report is a bit hidden in the issue summary, but chx explained this to me in irc.

- when you request system.date config in a drupal_get_user_timezone() call, it happens before the language system is bootstrapped, so the event listener runs but doesn't have a language available to override the configuration with.

- currently because of #1187726: Follow-up: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) this gets rebuilt every time, so it's rebuilt later in the request and the language system override actually starts working again - i.e. the lack of caching is hiding the bug.

- as soon as #1187726: Follow-up: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) is fixed, the override will never actually get a chance to run with a language. There may be other CMI files with the same issue (i.e. if anything in system.performance needed a language override).

- #1858984: Split system date formats from timezone settings is a workaround for all this since it splits the config file into 'early' and 'late' bootstrap phase (until you want to translate the system.timezone configuration then you're in a mess again).

If the language system moves into the DIC entirely and gets lazy initialized, then there won't be a language bootstrap phase, but I think there's still some problems thinking about this more:

- CMI can and does have to run super-early, before we have a container as such - i.e. you can't build the container without consulting CMI at the moment and for the forseeable future, so any application-level assumptions in CMI have to be transparent at this level - either not run or silently fail without causing race conditions, but we also shouldn't have to load config objects twice either.

- languages are going to move to config entities in #1754246: Languages should be configuration entities. When that happens I don't see how it'll be possible to translate language properties (like German into Deutsch using config overrides). Translation is going to depend on the event is going to depend on the current language is going to depend on CMI which depends on the event. This is going to be an issue if the translation is done before the configuration object is returned from the configuratoin API regardless of what that is though.

While there needs to be a discussion about events vs. hooks, I don't think this is the place to have it. A lot of the arguments about hooks around the place are because we didn't do #1331486: Move module_invoke_*() and friends to an Extensions class yet, and events /in Drupal/ depend on the module list, so the circular dependencies are still there at a high level even if it's a bit easier to keep them hidden in the code. All of this means there's really no difference between events and hooks in terms of the actual dependencies in the system for the purposes of this bug.

catch’s picture

Title: The config override concept is broken » Circular dependencies and hidden bugs with configuration overrides
Gábor Hojtsy’s picture

Any suggestions if neither event listeners nor hooks can be used?

Jose Reyero’s picture

Title: Circular dependencies and hidden bugs with configuration overrides » Issues with (upcoming) config caching, language initialization, and configuration overrides

@catch,
Yes, ok, I see the "future issue" once config is cached, thanks for taking the time of explaining it.

But really, this way of "prospective bug reporting" (A is broken if we commit B) looks weird to me. I thought the way we worked was adding the issue to B so it is fixed before the other is committed...

Anyway, getting to the "issue" this is how I see it:
- I'd rather have hooks instead of event listeners provided we find a way of initializing the whole thing properly.
- There's no solution for the "thing is not translated if used before language initialization" unless we want to do the language initialization before the config system, which I don't think we want.
- Also, since localization is a module we cannot pretend to localize stuff before module initialization unless we move the whole concept one level below, which I don't think we want either.

The easy fix IMHO: Flush the configuration cache right after modules are loaded (and module bundles get into the DIC)

Updated title, I think it explains the issue better.

catch’s picture

Title: Issues with (upcoming) config caching, language initialization, and configuration overrides » Circular dependencies and hidden bugs with configuration overrides

The question should be "how can we avoid a circular dependency?". Which particular systems get used for this is not the point. it's just a shame that event listeners are being sold as the solution to circular dependencies when they really solve next to nothing.

I had a long chat with beejeebus about this last night, but it's very tricky, however I think we got somewhere approaching the following:

- when loading configuration if we want overrides to be cacheable, we'd need to load all available configuration into an array - then this can be cached at any point in the request reliably since it's always the same. Config overrides as they are now would then be adding stuff (config additions?) to this array rather than actually replacing anything.

So this almost ends up as extra, non-final, storage controllers or similar but it could also stay as an event. The mechanism is less important than what it does. The main thing here is that during this step there is absolutely no request/context dependency at all so that results are reiable every time. This means more stuff to grab if you have lots of languages, but at least that's a linear problem.

Then separately, when requesting a configuration key, similar place to the context patch that's in the queue, there'd be something like a resolver. - i.e. find the best match key in the array based on some rule, same as language fallbacks. We may or may not be able to cache this (i.e. given langcode 'en' from wherever find me the best match configuration), but it'd probably have to run on each ->get($key) and caching is still hard because the context might change on it (although maybe DIC scoping helps there?).

This is extremely similar to how field/entity storage works now, except that's hardcoded to language (and we might end up having to do that in the end if we keep running into issues like this).

However even with those changes, it'd still mean that any configuration keys requested before (or during) the language being set up for the request won't be translatable.

Right now that includes any early bootstrap stuff, language negotiation settings and the languages themselves I think? If we only request untranslatable keys of those configuration objects then this might end up being OK - i.e. if you just need the langcode of a language but not the human readable name, and requesting the langcode only triggers translation of that, not the whole configuration object, then it matters less - i.e. the date/timezone config split wouldn't have to happen if that became possible.

Jose Reyero’s picture

Sorry, I didn't mean to change the new title, this one looks ok too.

catch’s picture

Issue tags: +Stalking Crell

I crossposted with Jose. The before/after is really the problem, I think it boils down to this.

The configuration system has a dependency on EventDispatcher.

EventDispatcher doesn't have many explicit dependencies, (hence why it's popular).

We let CoreBundle and modules-provided bundles register events.

In this case, the specific event listener added by the locale system depends on the configuration system.

Going to tag this with 'Stalking Crell' since I know he loves issues like this.

chx’s picture

Priority: Major » Critical
chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

chx marked #1877992: Restore sanity to CMI as duplicate however that documents another circular dependency - php file storage depends on CMI, which depends on the kernel, which depends on php file storage.

heyrocker’s picture

We should probably move the file paths out of config, that should have been thought of during the conversion. They seem very much like state to me, I've seen more than one situation where they differ between dev and live sites. Another alternative which chx brought up to me on IRC was that we just put it in settings.php, which I would also be fine with if we all agree that the UI can go away.

catch’s picture

settings.php works for me, we can put it in $settings per #1833516: Add a new top-level global for settings.php - move things out of $conf. I don't think we can put it in state - needs to be possible to keep it in revision control - even if that's settings.local.php.

heyrocker’s picture

Somehow that issue had completely eluded my attention. That's going to help out several things.

chx’s picture

Status: Active » Postponed (maintainer needs more info)

On settings_get from #15

chx’s picture

Status: Postponed (maintainer needs more info) » Postponed

grrr wrong status.

catch’s picture

Status: Postponed » Active

I don't see how that fixes the language dependencies since that's configured via the UI (and there was a plan put forward for this, by chx, in irc relating to storing the list of enabled languages separately from the languages themselves).

chx’s picture

Status: Active » Postponed

Language is resolved by the context patch. And of course settings()->get() won't resolve the conf dependency in itself but if we are to move file_public_path into settings, let's use a pretty api for it, no?

catch’s picture

Status: Postponed » Active

file_public_path is still in variable_get() - so if we move that to $settings what else is left here?

I know the language CMI patch went in with some workarounds but didn't get a chance to review that fully.

Either way this isn't postponed any more.

xjm’s picture

Let's try to incorporate the bug description from #4 into the summary.

xjm’s picture

Issue tags: +Needs tests

Also, let's make sure that this is still an issue by providing a failing test for it.

catch’s picture

catch’s picture

Issue summary: View changes

Updated issue summary.