Resetting an entity_load cache bin in Drupal 7 can have more drastic consequences than in previous versions of Drupal, as we can have persistent load caches in place of the PHP static memory cache.

Instead, wherever possible, only flush the necessary entities from the cache, using entity_get_controller($entity_type)->resetCache(array($entity_id));

In this case, the use of $reset in workbench_moderation_node_live_load() appears to be entirely redundant, as a revision id is being specified, and when loading a revision the entity load cache is not used at all (even if it is the current revision); the entity object is always generated freshly in that situation.

As such, I think we can simply remove the flag from that call.

n.b. Within this module, workbench_moderation_node_live_load() is only called by workbench_moderation_store(), which in turn is only called by workbench_moderation_moderate() via drupal_register_shutdown_function('workbench_moderation_store', $node).

Comments

jweowu’s picture

Status: Active » Needs review
StatusFileSize
new591 bytes
jweowu’s picture

And because I need it at the moment (and in case anyone else does too), here's a version of that patch which applies to version 1.2 of the module.

jweowu’s picture

Issue summary: View changes

Cross-referencing with core API issue

acbramley’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I've been using this patch for a long time on multiple projects with no side affects, marking RTBC

recrit’s picture

re-roll for latest 7.x-1.x

colan’s picture

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

Thanks!

  • colan committed 4b7abe8 on 7.x-1.x authored by jweowu
    Issue #1963992 by jweowu, recrit: Removed $reset flag with node_load().
    
acbramley’s picture

There's only a single node_load call in workbench_moderation on the 7.x-2.x branch, I think that means that the functionality this patch fixes would now be in state machine therefore we don't need to port a patch, but potentially open an issue there?

colan’s picture

Project: Workbench Moderation » State Machine
Version: 7.x-2.x-dev » 7.x-3.x-dev

Moved to the other queue. Close if not an issue.

jweowu’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new866 bytes

Yes, a grep of the code shows that this is also an issue here.

It's probably the same fix -- it's a revision which is being loaded in each case here, so simply removing the $reset = TRUE argument has no effect on the return values of those node_load() calls.

We need to confirm whether the other code is making assumptions about the rest of the entity load cache, however. If anything needs to be purged from the load cache, we need to do that manually with entity_get_controller($entity_type)->resetCache(array($entity_id));

I've not looked at state_machine before, so would appreciate input from someone who knows the code.

jweowu’s picture

Version: 7.x-3.x-dev » 7.x-2.x-dev

That patch is actually for the 7.x-2.x branch, and I can confirm that the bug doesn't exist in 7.x-3.x

jweowu’s picture