Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverting 'Published' post to 'Draft' or 'Pending Review' status does not clear the cache file #441

Closed
raamdev opened this issue Mar 13, 2015 · 13 comments
Milestone

Comments

@raamdev
Copy link
Contributor

@raamdev raamdev commented Mar 13, 2015

Steps to reproduce this bug

  1. Enable ZenCache and then visit a published post while logged out to generate a cache file for that page
  2. Login to the WordPress Dashboard and change the status of the post from "Published" to "Pending Review" or "Draft"

Expected Behavior

ZenCache should purge the cache file for that post so that it no longer appears on the site, as the new post status of "Pending Review" is not public.

Observed Behavior

The cache file remains and the post is still accessible for as long as the cache file exists.


Tested with ZenCache Pro v150218.


Other Notes

When the post has been changed from "Published" to "Pending Review" or "Draft", moving it to "Trash" also does not purge the cache file as expected.

However, moving a post from "Published" to "Trash" does purge the cache file as expected.

@raamdev raamdev added this to the Future Release milestone Mar 13, 2015
@raamdev raamdev changed the title Reverting 'Published' post to 'Pending Review' status does not clear the cache file Reverting 'Published' post to 'Draft' or 'Pending Review' status does not clear the cache file Mar 13, 2015
@jaswrks
Copy link

@jaswrks jaswrks commented Mar 20, 2015

Referencing this line where we need to add $new_status === 'pending' in order to fix this bug.

@jaswrks
Copy link

@jaswrks jaswrks commented Mar 20, 2015

I also suggest that we refactor this sub-routine. As it exists at the moment, this flag is not considered by the static object cache handler in the lines just above it.

Therefore, calling this routine indirectly with the flag off, and then calling it again later on a status transition, will result in the method not running again, when in fact it needs to; i.e., the parameters have changed.

So what needs to happen is that static::$static['___allow_auto_clear_post_cache'] needs to become one of the deciding factors in whether or not the method has already been called.

$___allow = !isset(static::$static['___allow_auto_clear_post_cache']) || static::$static['___allow_auto_clear_post_cache'] !== FALSE;
if(isset($this->cache[__FUNCTION__][$post_id][(integer)$force][(integer)$___allow]))
    return $counter; // Already did this.
$this->cache[__FUNCTION__][$post_id][(integer)$force][(integer)$___allow] = -1;

Better yet, I'd like to do away with this flag altogether. It seems unnecessary. In both cases where this flag is set to TRUE by another method, the call to clear the post cache is within the same method where this flag is set. Therefore, we could just as easily decide not to call auto_clear_post_cache(); instead of setting a flag that is picked up later.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 22, 2015

Referencing this line where we need to add $new_status === 'pending' in order to fix this bug.

Ah, yes, that looks like the issue. Thanks for catching that!

Therefore, calling this routine indirectly with the flag off, and then calling it again later on a status transition, will result in the method not running again, when in fact it needs to; i.e., the parameters have changed.

Ah, good catch! I see what's happening there.

Better yet, I'd like to do away with this flag altogether. It seems unnecessary. In both cases where this flag is set to TRUE by another method, the call to clear the post cache is within the same method where this flag is set. Therefore, we could just as easily decide not to call auto_clear_post_cache(); instead of setting a flag that is picked up later.

Hmm, I'm not so sure about that. What about where we hook into auto_clear_post_cache()? In those cases, a WordPress hook is firing auto_clear_post_cache(), so we need the check inside the routine itself.

If I recall correctly, this check exists inside the routine itself because certain hooks (e.g., clean_post_cache, if memory serves me correctly) fire multiple times during some events (such as post status transitions, if memory serves me again).

I remember spending a lot of time debugging stuff that was eventually fixed by adding that ___allow_auto_clear_post_cache flag, so I wouldn't want to drop it without extensive testing.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 22, 2015

Actually, ___allow_auto_clear_post_cache was used during transitions related to comments:

I believe clean_post_cache is fired during those transitions, which was causing auto_clear_post_cache() to fire unnecessarily. The ___allow_auto_clear_post_cache flag resolved that issue.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 22, 2015

Referencing GitHub Issue where a bug was fixed by adding ___allow_auto_clear_post_cache: #159

@jaswrks
Copy link

@jaswrks jaswrks commented Mar 22, 2015

@raamdev writes...

I believe clean_post_cache is fired during those transitions, which was causing auto_clear_post_cache() to fire unnecessarily. The ___allow_auto_clear_post_cache flag resolved that issue.

I see what you mean. In that case, I'm not sure that the current flag actually works as intended. More on this below.

@raamdev writes...

https://github.com/websharks/zencache/blob/000000-dev/zencache/zencache.inc.php#L1668-L1674
https://github.com/websharks/zencache/blob/000000-dev/zencache/zencache.inc.php#L1715-L1720

... Notice that we set the flag in each of these cases, BUT just 5 or 6 lines below it we call $counter += $this->auto_clear_post_cache.

So the flag is reset within the same method; i.e., we set the flag, then we actually call the method—knowing ahead of time that it will not run in certain cases, because we set the flag.

This is why I said...

we could just as easily decide not to call auto_clear_post_cache(); instead of setting a flag that is picked up later.


Given that PHP is a synchronous language, looking at an example like this where we call auto_clear_post_cache() after having set the flag to prevent another indirect call associated with a hook outside of this method, I'm not seeing how this flag is actually working as intended; even though I do see (remember) the reason for it to exist now.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 23, 2015

we set the flag, then we actually call the method

Uh, no, we don't.

    if($comment->comment_approved === 'spam' || $comment->comment_approved === '0')
                    // Don't allow next `auto_clear_post_cache()` call to clear post cache.
                    // Also, don't allow spam to clear cache.
                {
                    static::$static['___allow_auto_clear_post_cache'] = FALSE;
                    return $counter; // Nothing to do here.
                }

When we set static::$static['___allow_auto_clear_post_cache'] = FALSE, the very next line is return $counter;, so the lines below that are never executed.

@jaswrks
Copy link

@jaswrks jaswrks commented Mar 23, 2015

@jaswrks
Copy link

@jaswrks jaswrks commented Mar 23, 2015

What's worse is that I actually looked at this like five times. LMAO ~ It wasn't a quick glance and I missed it, my brain just told me that line was not there for some reason. haha ~ Age is kickin' in OMG.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 23, 2015

I'm not sure that the current flag actually works as intended.

I just retested this bug fix by commenting out these two lines anywhere inside zencache-pro.inc.php:

static::$static['___allow_auto_clear_post_cache'] = FALSE;
return $counter; // Nothing to do here.

I then left a comment on a post on my test install, made sure the home page was cached, and then reported my new comment as "Spam" from the WordPress Dashboard. Sure enough, the Home Page cache was cleared (unnecessarily). So, those lines are working as expected. :-)

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 23, 2015

What's worse is that I actually looked at this like five times. LMAO ~ It wasn't a quick glance and I missed it, my brain just told me that line was not there for some reason. haha ~ Age is kickin' in OMG

LOL. :-)

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Mar 28, 2015

Next release changelog:

  • Bug Fix: Fixed a bug where transitioning a Published post back to Pending or Draft would not automatically clear the cache file. This resulted in the post remaining accessible on the frontend despite being set as Pending or Draft. ZenCache now properly clears the cache file automatically when transitioning from Published to Pending or Draft, which prevents access to the post as expected. Issue #441.
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Apr 10, 2015

This fix has been released with ZenCache v150409:
http://zencache.com/zencache-v150409-now-available/

If you need to follow-up with something related to this GitHub Issue, please open a new GitHub Issue.

@wpsharks wpsharks locked and limited conversation to collaborators Apr 10, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants