Saving or updating a node in Drupal triggers cache_clear_all(NULL, 'cache_page'). However, node deletion does not. Instead, node_delete_multiple() calls

<?php
  entity_get_controller('node')->resetCache();
?>

which which clears a static cache but doesn't actually clear the cache in the database. It also fails to call

<?php
  _cache_get_object($bin)->clear($cid, $wildcard);
?>

The result is that third-party caching modules such as Redis may fail to properly clear cache upon node deletion and will therefore return stale content.

I noticed this issue while researching a problem with cache clearing in the Redis module, which I have reported separately:

http://drupal.org/node/1841878

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sheldon Rampton’s picture

Here's a patch:

Sheldon Rampton’s picture

Status: Active » Needs review
joshk’s picture

It might also want to clear the entity cache as well. That will prevent the $node object from appearing to continue to exist in later action in the same boostrap.

Sheldon Rampton’s picture

OK, here's an updated patch that also clears the entity cache.

olli’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Nice find. Was this already fixed in D8?

olli’s picture

Let's see. Moved cache invalidation to submit functions.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, node_deletion_clear_cache-1841900-6.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
ParisLiakos’s picture

Status: Needs review » Needs work

needs a reroll

olli’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

Here we go.

ParisLiakos’s picture

thanks!
just two nitpicks i could find.

  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodePageCacheTest.php
    @@ -0,0 +1,90 @@
    +  function setUp() {
    ...
    +  function testNodeDelete() {
    

    need visibility

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodePageCacheTest.php
    @@ -0,0 +1,90 @@
    +    \Drupal::config('system.performance')
    

    should use $this->container like:
    $this->container->get('config.factory')->get('system.performance');

olli’s picture

FileSize
877 bytes
4.43 KB

Thank you!

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks!
lets get it in!

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Nice find.

Committed f805ecf and pushed to 8.x. Thanks!

herom’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
2.33 KB
2.69 KB

backport patch to 7.x

Status: Needs review » Needs work

The last submitted patch, 15: drupal-1841900-15-tests-only.patch, failed testing.

ParisLiakos’s picture

hi herom, thanks for rolling this!

+++ b/modules/node/node.module
@@ -1286,6 +1286,7 @@ function node_delete_multiple($nids) {
+    cache_clear_all();

this call should happen in the form submit handlers not here, because, well, if you call that function many times programmatically, that would make things significantly slower;)

so this should go in node_delete_confirm_submit() and in node_multiple_delete_confirm_submit()

herom’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
1.94 KB

new patch.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks looks good!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.25 release notes

Status: Fixed » Closed (fixed)

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