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

Fix buggy auto-purge comment routines #237

Closed
wants to merge 2 commits into from
Closed

Conversation

raamdev
Copy link
Contributor

@raamdev raamdev commented Jul 11, 2014

  • Removed edit_comment and delete_comment action hooks from
    auto_purge_comment_post_cache(); these are now handled by the new
    transition_comment_status hook that is attached to the new
    auto_purge_comment_transition() routine.
  • Added a new auto_purge_comment_transition() routine; this new
    routine properly handles the transition from one comment status to
    another and only calls the purge routine when necessary.
  • A new static::$static['allow_purging'] variable tracks when a
    comment status change should not trigger a clear of the post cache
    and ensures that a subsequent call to auto_purge_post_cache() does
    not cause the post cache to be cleared. This is necessary because, for
    example, a spam comment will eventually trigger the clear_post_cache
    hook, which attaches directly to auto_purge_post_cache() and would
    result in the post cache being cleared even when the comment cache
    does not get cleared.

See #159


@jaswsinc Code review please, when you get a chance?

- Removed `edit_comment` and `delete_comment` action hooks from
`auto_purge_comment_post_cache()`; these are now handled by the new
`transition_comment_status` hook that is attached to the new
`auto_purge_comment_transition()` routine.

- Added a new `auto_purge_comment_transition()` routine; this new
routine properly handles the transition from one comment status to
another and only calls the purge routine when necessary.

- A new `static::$static['allow_purging']` variable tracks when a
comment status change should _not_ trigger a clear of the post cache
and ensures that a subsequent call to `auto_purge_post_cache()` does
not cause the post cache to be cleared. This is necessary because, for
example, a spam comment will eventually trigger the `clear_post_cache`
hook, which attaches directly to `auto_purge_post_cache()` and would
result in the post cache being cleared even when the comment cache
does not get cleared.

See #159
@jaswrks
Copy link

jaswrks commented Jul 12, 2014

Awesome! I am reviewing this and running some quick tests. There was one thing that troubled me, but it seems I overlooked that you are resetting the allow_purging flag on the next call. Aha! I missed that before. I'll report back in just a sec.

@jaswrks
Copy link

jaswrks commented Jul 12, 2014

Just ran several tests against this where a comment transitions from one status to another. Seems to be working phantasmagorically 👏 From your flowchart to completion, this was awesome work man!

Regarding the static::$static['allow_purging'] flag, there are some other routines that use static::$static[__FUNCTION__], so if we ever ended up with an allow_purging function that could be an accidental conflict. No big deal at all, but it might help to future-proof this by adding a special prefix to it (or something). Maybe static::$static['___allow_auto_purge_post_cache'].

@raamdev
Copy link
Contributor Author

raamdev commented Jul 13, 2014

Seems to be working phantasmagorically From your flowchart to completion, this was awesome work man!

Thanks! This was a PITA to get right.

it might help to future-proof this by adding a special prefix to it (or something). Maybe static::$static['___allow_auto_purge_post_cache'].

Done. Thanks!

@jaswrks
Copy link

jaswrks commented Jul 16, 2014

@raamdev writes... (regarding the latest RC release)

Unapproved comments that are marked as spam on my site are triggering auto-purge routines to unnecessarily clear cache files

I was unable to reproduce this on a standard WP install (3.9.1), running the latest Quick Cache Pro v140714 (RC). Marking an unapproved comment as Spam did not trigger a clearing of the cache.

@raamdev
Copy link
Contributor Author

raamdev commented Jul 16, 2014

@jaswsinc Thanks for testing and for the confirmation! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants