Container cannot be saved to cache

Note this is just a notice, not an error or a warning.

    // If needs dumping flag was set, dump the container.
    if ($this->containerNeedsDumping && !$this->cacheDrupalContainer($container_definition)) {
      $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be saved to cache.');
    }

Well, "just a notice", but its a fundamental problem when a cache set fails, isn't it? Well, its way less horrible but if this is constantly the case, the site slows down a lot.

Oh, you're right! I didn't read the surrounding code. My bad.

Why is that even a notice and not at least a warning?

Also, the lack of an explicit reason makes it difficult to fix. Bumping to major.

Well the reason is there, the cache set call failed ... throwing failures in cache->set() seems wrong, because it is currently designed to not do so.

But yeah 'notice' is the wrong level for me, error describes it better ...

This message appears every time update.php is run.

In fact the container should not be cached in update.php but this should not be logged as a notice/warning/error. This is not Major because nothing wrong happens, it's just a false positive.

@claudiu.cristea But then this is a total different issue than what the actual problem is here, right? Your patch is about not saving it on updates,
but the issue here is that something is going on write doing that. I'm pretty sure that this just depends on actual triggering it, rather than the fact that update.php is triggering it.

@dawehner, because UpdateKernel::cacheDrupalContainer() returns FALSE every time, the next if() is FALSE TRUE (edited) in DrupalKernel when running update.php:

    // If needs dumping flag was set, dump the container.
    if ($this->containerNeedsDumping && !$this->cacheDrupalContainer($container_definition)) {
      $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be saved to cache.');
    }

So, the notice is raised even we don't want to cache, as is the case of update.php. Note that all the complains in this ticket are from update.php.

With the patch at #9 applied to 8.0.2 the message no longer appears when update.php is run.

@claudiu.cristea
Oh I see!

+++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
@@ -43,16 +43,10 @@ protected function initializeContainer() {
+    $this->allowCaching = FALSE;
+    return $container;

What about overriding $allowCaching on the base level of the class?

@dawehner, You mean in this way?

I would not pass it in the constructor signature because this would mean changing the signature. Hm, yes, that can be done with an optional param, at the end. Opinions?

Worked on some test coverage and realized we can solve this in a different way

Didn't posted an interdiff, because nothing stays the same.

Is containerNeedsDumping the same as "can be cached"? I had the same idea at the beginning but, at least semantically, I felt that they are different things.

@claudiu.cristea
Yeah I can see what you mean.
So to be clear its an implementing detail that dumping a container is caching now, before we had our own container, we just had container dumping,
so we either want that, or we don't want to, which is the case for the update.php one.

@dawehner, I agree with #17 considering the actual update.php. But what if we'll want to cache the container in update.php in the future? I think update.php is a multi-request process, right? So, why not caching the container to allow the batch requests to be faster? The leakage concern can be fixed easily with a good cache id. I don't want to say that is for this issue, I just wanted to let the door open for a future caching.

Well, actually, the idea to not cache it for update.php is explicit, because we wan't to have a system with as few dependencies as possible, so for the container it would be a running
cache system. IMHO requiring it, or at least make it a little bit more bound to it, would be bad.

Optimizing for the speed of update.php is IMHO pretty pointless, especially because its major problem is scalability.

OK. Let's, then, increase the alert level to error() for future cases, as per #5 and #7.

In the meantime, site admins running update.php are receiving notices making them, probably, nervous. Let's push this in :)

Oh yes, good point. Notices are especially ignored by syslog by default if I remember correctly, which is a problem of course.

Looks great to me.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

I also face this issue in the Drupal8.5.1 version of journal eight themes.
Also, newly created blocks for the website are missing which includes google map.

Thanks

FYI, I finally found this bug and made the corresponding change in Drush. WIll be in next Drush release. -