New to Drupal. Not a developer or programmer. Upgraded to 8.0.2 through Softaculous. No problem during install of upgrade. Recent Logs shows three entries repeating this message:

Type DrupalKernel
Date Thursday, January 7, 2016 - 17:43
User xxxx (me)
Location http://cosmastrologia.com/update.php/selection
Referrer http://cosmastrologia.com/update.php
Message Container cannot be saved to cache.
Severity Notice
Hostname 184.158.80.117

If this is not the place to post this, will someone direct me to the right place? Apologies if I did not make correct selections. I am trying. Thank you.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Spoon Fed World created an issue. See original summary.

Spoon Fed World’s picture

"Container cannot be saved to cache" continues to appear in Recent Logs.

Wim Leers’s picture

Component: cache system » base system
Assigned: Unassigned » Fabianx

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.');
    }
dawehner’s picture

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.

Wim Leers’s picture

Version: 8.0.2 » 8.0.x-dev
Priority: Normal » Major

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.

dawehner’s picture

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.

dawehner’s picture

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

serg2’s picture

This message appears every time update.php is run.

claudiu.cristea’s picture

Priority: Major » Normal
Status: Active » Needs review
FileSize
1.86 KB

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.

dawehner’s picture

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

claudiu.cristea’s picture

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

serg2’s picture

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

dawehner’s picture

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

claudiu.cristea’s picture

FileSize
2.14 KB
996 bytes

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

claudiu.cristea’s picture

Assigned: Fabianx » Unassigned
dawehner’s picture

Assigned: Unassigned » dawehner

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

dawehner’s picture

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

claudiu.cristea’s picture

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.

The last submitted patch, 17: 2646410-17-fail.patch, failed testing.

dawehner’s picture

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

claudiu.cristea’s picture

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

dawehner’s picture

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.

claudiu.cristea’s picture

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

claudiu.cristea’s picture

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

dawehner’s picture

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

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 9a2b588 on 8.1.x
    Issue #2646410 by claudiu.cristea, dawehner: Container cannot be saved...

  • catch committed fbee075 on 8.0.x
    Issue #2646410 by claudiu.cristea, dawehner: Container cannot be saved...

Status: Fixed » Closed (fixed)

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

sarikak’s picture

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

moshe weitzman’s picture

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