CPS took over the hooks from SPS, who relied on a non-applied entitycache patch (without saying so).

This fixes the entitycache integration using #2044291: Add drupal_alter() to entityCacheGet & entityCacheSet & resetEntityCache and also fixes the annoying case that cps_override_changeset() actually clears the whole entitycache via resetCache().

By carefully avoiding that non published items enter the cache its possible to skip clearing the entitycache on override changeset and only clear the static cache.

CommentFileSizeAuthor
#11 interdiff.txt360 bytesDavid_Rothstein
#11 fix_cps_entitycache-2784487-11.patch5.15 KBDavid_Rothstein
#9 interdiff.txt1.05 KBDavid_Rothstein
#9 fix_cps_entitycache-2784487-9.patch5.1 KBDavid_Rothstein
#6 interdiff.txt441 bytesDavid_Rothstein
#6 fix_cps_entitycache-2784487-6.patch4.89 KBDavid_Rothstein
#3 interdiff.txt4.05 KBfabianx
#3 fix_cps_entitycache-2784487-3.patch4.86 KBfabianx
#2 fix_cps_entitycache-2784487-2.patch3.11 KBfabianx

Comments

Fabianx created an issue. See original summary.

fabianx’s picture

fabianx commented
Status: Active » Needs review
Related issues: +#2044291: Add drupal_alter() to entityCacheGet & entityCacheSet & resetEntityCache
StatusFileSize
new fix_cps_entitycache-2784487-2.patch3.11 KB

Attached patch fixes the problems for me.

fabianx’s picture

fabianx commented
StatusFileSize
new fix_cps_entitycache-2784487-3.patch4.86 KB
new interdiff.txt4.05 KB

The publishing needed a complete disable as well - especially as drafty() did a entity_load_single() after the resetCache() call. (and the resetCache() itself would be a performance bottleneck as well as it deletes all cache entries for every entity published).

Interdiff attached as well.

Status: Needs review » Needs work

The last submitted patch, 3: fix_cps_entitycache-2784487-3.patch, failed testing.

The last submitted patch, 3: fix_cps_entitycache-2784487-3.patch, failed testing.

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Needs review
StatusFileSize
new fix_cps_entitycache-2784487-6.patch4.89 KB
new interdiff.txt441 bytes

In the previous patch, the code in cps_override_changeset() wasn't actually restoring the previous state - so the entitycache reset functionality was staying disabled for the rest of the page request after that function was called.

Here's a fix for that.

fabianx’s picture

fabianx commented

#6: Are you sure about this?

I kinda kept NULL as the state and that worked fine for me as all code was only checking if (x) and if (NULL) is the same as if (FALSE).

However not opposed to making the default FALSE, should then be done for cps_entitycache_disable() as well though.

fabianx’s picture

fabianx commented

Oh oh, I see the problem now.

Because the value was NULL, it tried to reset with NULL and that failed as the isset() check was FALSE.

That cannot happen for the other one as that uses direct TRUE / FALSE instead of retrieving the old value.

David_Rothstein’s picture

David_Rothstein commented
StatusFileSize
new fix_cps_entitycache-2784487-9.patch5.1 KB
new interdiff.txt1.05 KB

Right, exactly.

Here's an untested reroll that changes cps_disable_entitycache() to have a default of FALSE also. It doesn't matter in practice right now (for the reason you mentioned above) but I think you're right that it's good to have them be consistent, especially if anyone ever does try to feed the return value of cps_disable_entitycache() back into it sometime in the future.

fabianx’s picture

fabianx commented 
+++ b/cps.module
@@ -2069,6 +2107,51 @@ function cps_move_changeset($changeset_from, $changeset_to, $entity_type, $entit
+ * @return bool
+ *   The old state that this function had before, or FALSE if the cache was
+ *   never previously disabled.

That however is not true.

I think it might be best to just have the disable entitycache function and have it return something.

As the reset is just used for something that calls resetCache(), which could also disable all of entitycache instead.

---

If I were to rework I would probably do it like this:

$old_state = cps_disable_entitycache();

cps_disable_entitycache(TRUE);

cps_disable_entitycache(FALSE);

cps_disable_entitycache($old_state);

That solves all cases and also ensures whatever someone wanted as state before is preserved (for recursive calls and such).

David_Rothstein’s picture

David_Rothstein commented
StatusFileSize
new fix_cps_entitycache-2784487-11.patch5.15 KB
new interdiff.txt360 bytes

Oh, you're right - I copied the documentation but not the code. Here is another untested reroll that just copies the code over so the two functions behave identically. No other changes.

David_Rothstein’s picture

David_Rothstein commented

In other words I didn't change the calling code at all, although you're probably right that it shouldn't call cps_disable_entitycache(FALSE) indiscriminately at the end.

fabianx’s picture

fabianx commented

Yes, that is what I meant.

It should call it, but then restore the old state (whatever that is).

Also I question (myself) whether we really want two functions given that we only call resetCache() during the disabling of the cps_disable_entitycache_reset().

So it might be enough to just disable entitycache completely during that operation ... - instead of having two functions.

hadsie’s picture

hadsie commented
Status: Needs review » Fixed

I've committed the patch in #11 now. Thanks!

Status: Fixed » Closed (fixed)

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