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

Add method for purging all post terms #117

Merged
merged 5 commits into from
Apr 24, 2014
Merged

Add method for purging all post terms #117

merged 5 commits into from
Apr 24, 2014

Conversation

raamdev
Copy link
Contributor

@raamdev raamdev commented Apr 23, 2014

@JasWSInc Could you review this for me?

  • Adds method for purging all post terms.
  • Merges the latest code from 000000-dev into the purge-post-terms feature branch.
  • Fixes the E_NOTICE-level errors triggered by unavailable change_notifications_enable option.

@jaswrks
Copy link

jaswrks commented Apr 23, 2014

Awesome! I will test in just a moment.

@jaswrks
Copy link

jaswrks commented Apr 23, 2014

This is working great! Just tested it with Posts and also with a custom post type.

One suggestion...

  • There is just one dashboard notice for things like this (which I agree with). However, there could actually be several different terms purged by this new routine.

Here are some examples of the notice that I receive in the dashboard.

2014-04-23_14-57-31

2014-04-23_15-02-57


The problem? Not really a problem. However, I think this notice could be more accurate, even if that means being less specific if we need to. This really should tell me something like, "found archive views; i.e. categories, tags and/or other terms".

For instance, in the second example above there were actually three categories and two tags purged. Not just the one specific category that the notice listed for me.

@raamdev
Copy link
Contributor Author

raamdev commented Apr 23, 2014

The problem? Not really a problem. However, I think this notice could be more accurate, even if that means being less specific if we need to. This really should tell me something like, "found archive views; i.e. categories, tags and/or other terms".

Interesting. I had not tested that but I thought, looking at the code, that it would display several notices for each one it was clearing. I'll take another look at this and get that fixed, as I agree it should definitely be more clear about that.

@jaswrks
Copy link

jaswrks commented Apr 23, 2014

Here's a thought...

Most of the purging routines for Posts have the !empty($notices) check inside the loop to prevent repeated lines in the notice. This prevents QC from listing all of the specifics and cluttering the site owner's workstation when they receive these dashboard notices.

However, I wonder if that's wrong? Maybe a better solution would be to show only the first line, but add some HTML/JavaScript to toggle the remaining messages?

There probably should still be a limit though. Like, no more than 100 lines (ever).

@raamdev
Copy link
Contributor Author

raamdev commented Apr 23, 2014

Interesting. I totally missed that line !empty($_notices) line in the loop. So I guess that means currently only one notice will ever be created for each type of purge? That definitely seems wrong.

However, I can see why it wasn't a problem until now: you'll only have 1 Home Page cache to clear, 1 Posts Page cache, and 1 Post Cache. Now that we're adding support for Tags and Categories (and other archive views, like Author Archives, eventually), we'll definitely need a better way of handling this.

Maybe a better solution would be to show only the first line, but add some HTML/JavaScript to toggle the remaining messages?

I think that's a great idea! The message could say "Quick Cache: detected changes. Purging cache files. (click to see details)". Clicking to see the details would open a DIV below that which includes a log of sorts showing what types of cache files were created (essentially just the same messages we have now, minus the "Quick Cache: detected changes." part).

I've never done a toggle inside a Dashboard message like that. Have you?

@jaswrks
Copy link

jaswrks commented Apr 23, 2014

I've never done a toggle inside a Dashboard message like that. Have you?

Great! Well, WP loads jQuery in the dashboard so you can depend on that.
See jQuery.toggle() http://api.jquery.com/toggle/

Conflicts:
	quick-cache/quick-cache.inc.php
	Resolved conflict by adding auto_purge_post_terms_cache()
@raamdev
Copy link
Contributor Author

raamdev commented Apr 24, 2014

@JasWSInc I'm marking this feature as done and merging it into 000000-dev. I updated auto_purge_post_terms_cache() to support up to 100 notices (e350503), so at least this branch is feature-complete.

I'll work on the Compact Dashboard Notices (#118) feature separately.

@raamdev raamdev merged commit b4dc52c into 000000-dev Apr 24, 2014
raamdev added a commit to wpsharks/comet-cache-pro that referenced this pull request Apr 24, 2014
@jaswrks
Copy link

jaswrks commented Apr 24, 2014

Awesome!

@sarangandk
Copy link

When will this change released? I too had this problem in a long time in Quick Cache Pro version.

@raamdev
Copy link
Contributor Author

raamdev commented Apr 29, 2014

@sarangandk It's coming within the next week or two. In the meantime, if you're interested in testing a beta release of Quick Cache before the next version comes out, please sign-up to be a beta tester here.

@raamdev raamdev deleted the purge-post-terms branch May 22, 2014 21:57
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

3 participants