Calendar: Object cache not cleared

[asadowski10]

Hi team,

Since the 0.8.3 release and the feature > Improvement: Cache calendar items for each user individually to prevent potential cache pollution. Props justnorris.
We are having cache problems.
When we update content, the cached version is not updated.

The cache key is based on the user_id ( $cache_key = $post->ID . $can_modify . '_' . get_current_user_id(); ) but when you delete the cache , the key has no user_id ($post_id . 'can_modify' )
https://github.com/Automattic/Edit-Flow/blob/master/modules/calendar/calendar.php#L1800

Could you delete cache for user or remove user_id in cache key to have cache for all users ?

Thanks a lot

[mjangda]

The change was introduced here #412 to make the cache user-specific. Removing the user ID would re-introduce the bug it was trying to fix (#252).

Two thoughts:

• Find a way to clear the cache for all users (otherwise we risk showing stale data to some).
• See if we can safely remove the HTML fragment caching and avoid all this complexity.

[jerclarke]

Wow this is a real nightmare to have gone on for so long. Found this ticket after persistent complaints from our sub-editors about the calendar being wrong in very random ways, and now it all makes sense.

To clarify the form this takes for anyone not familiar: The calendar item for a specific post will get trapped with a certain version of the metadata for the post (title+status) even after the post is edited. Notably the date of the post is not effected. If you drag a post to a new date, or change the date in the post editor, then reload the calendar, the item will show on the correct calendar day. BUT if you change the status or the title, it will show on the new date with the old title/status.

After setting up two windows with different users to confirm this, I commented out the lines that return the cached value in generate_post_li_html(), reloaded the calendar, and the data was all immediately correct.

Very chaotic! No wonder I had trouble pinning down exactly what was wrong!

Steps to reproduce:

• Log in as a user that can edit all posts
• In a private browser, log in as another user that can edit all posts
• Open the calendar in both windows (I believe it will be accurate at this point)
• Edit a post to have a different title and status in one of the windows
• Reload both windows: They both have the old title and status for that post.

I'm going to try to figure out a solution to purge the cache for everyone, or else I might just disable the cache entirely, as it seems like it might be more effort than it's worth.

Anyone know what about this generate_post_li_html() is so desperately in need of caching that it's worthwhile to cache nonces?

It seems to me that in the absence of an actually working cache-invalidation strategy, 100% of users will be better off without the cache. If they use it often, this bug is devastating. If they don't use it often, the cache has no meaningful effect.

[jerclarke]

OK FWIW after some testing I see that the performance is indeed pretty awful without this caching. Loading a Calendar date representative of how our editors would normally see it took 3s rather than 1s after disabling use of the cached values at the top of generate_post_li_html().

I'm still going to disable it for now, because accuracy is more important than speed, but clearly something is worth rescuing from this mess.

Worth noting is that according to Query Monitor, the number of actual DB queries doesn't go up when disabling the per-user cache, it just takes forever. Is it possible that it's not actual queries, but rather just PHP processing of the HTML, that takes forever?

It sure is a shame that we can't run the equivalent of wp_cache_delete_group(), as that would allow us to make the group ID identify a specific post, then delete the group to remove the cached results for all users simultaneously. Seems this obvious need just isn't met by the object caching API (apparently wp cli supports it, which isn't helpful to us).

I guess the approach of @cojennin is the best option. Figure out what exactly makes it slow, isolate that from anything that's per-user, and cache those things as user-independent values that only relate to the $post_id, so that we can clear them reliably on the clean_post_cache for that post.

I'll try to take a look at the old patch from cojennin and see about updating it. If someone could indicate that they are still able to merge a theoretical patch in the future, that would be a good motivation.

[jerclarke]

Ok so above you can see a refreshed version of Cojennin's patch. I made sure to integrate the various security improvements that had been made to calendar.php in the meantime. I didn't include the small change he had made to notifications, since they aren't directly relevant to this bug.

From my testing, the calendar works correctly with this patch. I checked that the object caching was operating as it should.

That said, I don't think this is a great solution because it doesn't actually help performance. I suspect that's what Mjangda was alluding to with his comments.

On my local site, the old caching brought generation time from 3s to 1s. With this caching, there's no noticeable difference at all, on or off the results were between 3.09 and 3.11 seconds to load a not-particularly-heavy calendar.

It seems that, probably due to object caching in core WP, the "queries" saved by caching the results from ->get_post_information_fields() just don't matter, probably because they are already cached.

So the question is: What is actually making it so slow, and can we cache THAT instead, without including any user-specific info that would make it impossible to invalidate the caches.

I'm going to take a stab at profiling the rest of the HTML generation to see if I can find a section that is the real culprit.

[jerclarke]

I realize I'm talking to myself here, maybe no other human will ever read these words, but I think I found the answer, at least for me, and it's cringe AF.

• The slowdown, at least for me, was coming from ->get_inner_information(), which generates the HTML of each post in the calendar.
• I narrowed it down to the contents of tr.item-field, then further down to the calling of $this->get_editable_html(), the removal of which gave the same benefits (1s instead of 3s) as the old caching.
• ->get_editable_html() has no documentation, which is a red flag, then upon investigating, it seems to be extremely vestigial and useless.
• In my case, the output of ->get_editable_html() was a completely deranged wp_dropdown_categories() being loaded individually for each and every post in the calendar. This was singlehandedly tripling the load time of the calendar. Our site has a LOT of categories (every country in the world) so in our case this is extra-dramatic.
• ->get_editable_html() is triggered when one of the fields from ->get_post_information_fields() has the editable value set to true.
• As far as I can tell, the only field that can come back as editable=true is taxonomies, even though ->get_editable_html() seems to have "support" for a variety of other $types like location/number/date.
• Importantly, the effect of this "editable taxonomy" is completely broken AFAICT. When in the calendar, after opening the info panel for a post and seeing the list of categories, I can click on a category to show the wp_dropdown_categories() output, a pulldown menu which has "General" selected and no way to save your choice, not that you would want to, considering that it only lets you pick a single option and that's not how categories work:

Overall I think I've discovered a hidey-hole with a mess someone forgot to clean up. AFAICT none of the elements in that little preview box are supposed to be editable at this point, the only thing you can change inside the calendar is the date, and that's by dragging to a different day.

I suspect that what's called for here is to remove the editable flag and all associated code entirely, at which point we can also remove all the caching rather than re-creating it.

For now I am going to give my own site's calendar a 3x speedup by just disabling the editable flag on taxonomies in get_post_information_fields():

if( current_user_can( $ed_cap, $post->ID ) )
	// $information_fields[$key]['editable'] = true;
	$information_fields[$key]['editable'] = false;

(This should also be doable with a filter on ef_calendar_item_information_fields, where you loop through and make sure editable is always false.)

I don't think I'm going to prepare an elaborate patch to remove the editable mess unless someone indicates they might merge it for me if it worked. For my own sake, or someone who wants to make this right later, I think these are the steps that would be needed:

• Remove ->get_editable_html()
• Clean up ->get_post_information_fields() to remove all reference to editable
• Clean up ->get_inner_information() to remove all reference to editable
• Clean up calendar.js and remove all reference to editable
• Clean up calendar.css and remove all reference to editable
• And most importantly: Remove caching from the calendar!

To me that last one is the most important. We should disable this cache ASAP because it's making the calendar unreliable, then clean up the performance as a separate task.

[jerclarke]

For anyone else who wants to speed up their calendar, object cached or not, here is a filter that fixes the "editable taxonomy" bug and which should be harmless if the bug is ever fixed in the core plugin:

/**
 * Edit Flow: Fix a ridiculous bug that causes an invisible wp_dropdown_categories() to be inserted in every calendar post
 * 
 * This filters a list of fields shown for each post by the Calendar module, and ensures they are not "editable".
 * 
 * See the Github issue below for details, but basically there's a whole 'editable' system in Edit Flow
 * which is vestigial and useless at this point, but the code lingers and has the effect of inserting
 * hidden "editable" fields in the calendar item for each post. 
 * 
 * This speeds up the calendar 3x, so about 1s rather than 3s. 
 *
 * @link https://github.com/Automattic/Edit-Flow/issues/505#issuecomment-3226002443
 * 
 * @param array $information_fields
 * @param int $post_id
 * @return array
 */
function gv_editflow_filter_ef_calendar_item_information_fields_to_disable_editable_fields(array $information_fields, int $post_id) {

	foreach ($information_fields as $key => $field) {
		if (isset($information_fields[$key]['editable'])) {
			unset($information_fields[$key]['editable']);
		}
	}

	return $information_fields;
}
add_filter('ef_calendar_item_information_fields', 'gv_editflow_filter_ef_calendar_item_information_fields_to_disable_editable_fields', 10, 2);

The only effect will be that the wp_dropdown_categories() isn't inserted as a hidden field in each post.