Problem/Motivation

The TMGMT tests are broken after #2584603: PHP exception on manage fields after enabling Configuration Translation because empty configuration is loaded as a config entity and then the field config system fatals:

Fatal error: Class name must be a valid object or a string in /usr/local/var/www/d8/www/core/lib/Drupal/Core/Field/FieldConfigStorageBase.php on line 31

The problem is that a ConfigFactory::get() implicitly creates new object and puts them in the static cache. And then when you call getMultiple() for the same object, it returns that and then everything explodes.

We had this for months if not years and so far didn't notice it because we usually don't mix single load and multiple load. We use the first for simple config and the second for config entities in almost all cases. But NodeTypeConfigMapper now breaks that by calling directly into the config factory for a config entity to see if it exists. I'm now sure why it does that, honestly, probably to access the config name?

Either way, this is definitely a wrong behavior and it's a bad one. Raising to major and moving to configuration system.

Proposed resolution

Luckily, the fix is easy. Just not sure which of the following approaches we should pick:
a) *not* store new objects in the static cache. Downside is that accessing the same non-existing object would be slower since we'd re-create it every time.

b) in doLoadMultiple(), check if object stored in the static cache is not new and only use it then. This needs a bit more work in doGet() or we still don't re-use it.
c) let callers deal with this. Not really an option IMHO but here it is, for completeness.

Attached are patches for both a) and b). I think I prefer a, I don't think we should store them in the static cache. Working on tests now.

Remaining tasks

Commit

User interface changes

None

API changes

None

Why this should be an RC target

Without out this patch ConfigFactory::loadMultiple() can return configuration that does not exist. This is a severe and unexpected bug.

Comments

juanse254 created an issue. See original summary.

juanse254’s picture

Status: Active » Needs review
FileSize
874 bytes

This should help fix it, feedback appreciated.

Status: Needs review » Needs work

The last submitted patch, 2: Config_missing-2598232-2.patch, failed testing.

tstoeckler’s picture

Wow, yes it would be very interesting to find out how that can happen. I will try to have a look at the TMGMT tests to see how I can reproduce the issue.

Not sure about the fix itself, loadMultiple() actually shouldn't be returning any empty objects, so if there's objects that are non-empty, but are missing an ID key, then something else very strange is going on.

Berdir’s picture

Yeah.. that seems like we need to figure out what kind of object is saved, could very likely be a tmgmt problem (or one in core that is uncovered by our test scenarios).

Berdir’s picture

Component: language system » configuration system
Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +rc target triage, +Needs tests
FileSize
1.57 KB
1.28 KB

Ok, tracked down the root cause for this.

The problem is that a ConfigFactory::get() implicitly creates new object and puts them in the static cache. And then when you call getMultiple() for the same object, it returns that and then everything explodes.

We had this for months if not years and so far didn't notice it because we usually don't mix single load and multiple load. We use the first for simple config and the second for config entities in almost all cases. But NodeTypeConfigMapper now breaks that by calling directly into the config factory for a config entity to see if it exists. I'm now sure why it does that, honestly, probably to access the config name?

Either way, this is definitely a wrong behavior and it's a bad one. Raising to major and moving to configuration system.

Luckily, the fix is easy. Just not sure which of the following approaches we should pick:
a) *not* store new objects in the static cache. Downside is that accessing the same non-existing object would be slower since we'd re-create it every time.
b) in doLoadMultiple(), check if object stored in the static cache is not new and only use it then. This needs a bit more work in doGet() or we still don't re-use it.
c) let callers deal with this. Not really an option IMHO but here it is, for completeness.

Attaching a batch for both a) and b). I think I prefer a, I don't think we should store them in the static cache. Working on tests now.

Berdir’s picture

Title: Sometimes config is missing from entity and this causes several errors » ConfigFactory::get() pollutes ::loadMultiple() static cache with new config objects
Issue summary: View changes

And a better title and issue summary.

The last submitted patch, 7: config-factory-static-cache-pollution-a-2598232-7.patch, failed testing.

The last submitted patch, 8: config-factory-static-cache-pollution-a-2598232-8.patch, failed testing.

Berdir’s picture

"Interesting."

That test relied on the fact that new objects are kept by reference and are updated, this is no longer the case now. Fix is easy but shows that there is a small behavior change with option a).

alexpott’s picture

This issue makes me sad. We broke this in #2395515: Config static cache is not cleared properly on delete

alexpott’s picture

let's do option b considering this is exactly the way we used to work.

Berdir’s picture

Works for me as well. Here's a small optimization not sure if we want to add that. I also thought about statically caching the information that we have a miss, we could store FALSE or NULL in the array, not sure how often that happens, I guess not so much? Separate issue anyway.

Both this patch and the one in #8 should be good to go I think?

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This looks great - nice to fix this in a way which such little impact. I'd love to make it so that ConfigFactory::get() does not create new objects but ho hum that hips has sailed.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue tags: -rc target triage +rc target

Discussed with with @xjm and we agree that this should be an rc target given the nature of the bug. Imo this is quite close to being critical.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Hmm I think I'd go back to #13 here, it's strange to statically cache something that's not loaded in the same set as things that were loaded.

If we stick with #16 here's a suggested interdiff because I found the comment hard to read:

diff --git a/core/lib/Drupal/Core/Config/ConfigFactory.php b/core/lib/Drupal/Core/Config/ConfigFactory.php
index a849616..6a4637d 100644
--- a/core/lib/Drupal/Core/Config/ConfigFactory.php
+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -161,9 +161,9 @@ protected function doLoadMultiple(array $names, $immutable = TRUE) {
     foreach ($names as $key => $name) {
       $cache_key = $this->getConfigCacheKey($name, $immutable);
       if (isset($this->cache[$cache_key])) {
-        // If the config objects exist in the cache, always unset it, but only
-        // use it if it is not new. We don't need to try and load a config
-        // object that we know doesn't exist.
+        // If the config obect is in the cache, always unset it from the list of
+        // items to load. If the config object is new, don't return it, because
+        // it hasn't yet been saved to storage.
         unset($names[$key]);
         if (!$this->cache[$cache_key]->isNew()) {
           $list[$name] = $this->cache[$cache_key];
alexpott’s picture

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

I'm fine with going back to #13. The reason I plumped for option B originally is cause RC and trying to minimise any behaviour change but thinking about it some more the change to Drupal\config\Tests\ConfigOverrideTest shows that this probably is a sensible decision as working with both mutable and immutable config at the same time should be extremely rare. In fact, what I really would like is for ConfigFactory::get() to not create new objects - but this is another issue.

Reuploading #13 - with a new name - it is good to go. Given the obscurity of the behaviour change I don't think a CR is worth it.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • catch committed 9980bd0 on 8.0.x
    Issue #2598232 by Berdir, alexpott, juanse254: ConfigFactory::get()...

Status: Fixed » Closed (fixed)

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