ConfigFactory::get() pollutes ::loadMultiple() static cache with new config objects

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.

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

This could be related to #2595535: Show helpful message (do not fatal!) when configuration files have different source language codes and cannot be translated

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.

And now tests.

And a better title and issue summary.

"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).

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

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

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?

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.

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.

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];

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.

Committed/pushed to 8.x, thanks!