-
Notifications
You must be signed in to change notification settings - Fork 11
Add support for wp embed cache clear --all command
#74
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
base: main
Are you sure you want to change the base?
Conversation
- Removes posts (`oembed_cache`), post metas (`_oembed_{hash}`, `_oembed_time_{hash}`), transients (`_transient_oembed_{hash}`, `_transient_oembed_time_{hash}`) caches
- Add tests
|
A couple of notes on this PR:
|
It's a problem when using an external object cache, because then transients will be in the object cache and not in the database. |
|
Hello @swissspidy, Thanks for the prompt reply.
I'm not sure to get what you meant here 😅
|
Whoops, it actually does but just in a form of a warning message: |
|
@swissspidy @danielbachhuber friendly bump 🙂 Anything I can do to help? |
danielbachhuber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nlemoine I think we need to loop and use the relevant WordPress functions. This will make sure the relevant core hooks get called (which plugins, etc. might be listening on), as well as make sure the post cache (clean_post_cache()), etc. is cleared.
Use WordPress functions to delete various oembed caches
|
Thanks @danielbachhuber for you feedback. I changed the way caches are deleted, I'm a bit afraid this will be super slow when dealing with a lot of data. Any thoughts on using a MySQL REGEXP query that only targets
Should I also introduce a |
danielbachhuber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making good progress! Nice work, @nlemoine
I changed the way caches are deleted, I'm a bit afraid this will be super slow when dealing with a lot of data.
Yep, such are things sometimes.
Any thoughts on using a MySQL REGEXP query that only targets
_oembed_{md5_hash}? Although it's more accurate, WordPress is considering any string starting with_oembedas an oembed cache:
Could you share more details about what you're thinking about?
Should I also introduce a
wp embed cache trigger --allcommand to warm all oembed caches?
Could! Feel free to open a separate GitHub issue and we can discuss how it might work.
| Then STDOUT should be: | ||
| """ | ||
| Success: Cleared 3 oEmbed caches: 1 post cache, 1 oembed post cache, 1 transient cache. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add some similar-looking meta, and then include verification that it doesn't get unexpectedly deleted?
|
|
||
| WP_CLI::success( $message ); | ||
| } else { | ||
| WP_CLI::error( 'No oEmbed caches to clear!' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| WP_CLI::error( 'No oEmbed caches to clear!' ); | |
| WP_CLI::error( 'No oEmbed caches to clear.' ); |
Should this be WP_CLI::success()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently is WP_CLI::error();, I stuck to what's in place right now.
But agree on the change. Makes more sense. You tell me.
| WP_CLI::error( 'You cannot specify both a post id and use --all' ); | ||
| } | ||
|
|
||
| // Delete all oEmbed caches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is superfluous
| // Get post meta oEmbed caches | ||
| $oembed_post_meta_post_ids = (array) $wpdb->get_col( | ||
| "SELECT DISTINCT post_id FROM $wpdb->postmeta | ||
| WHERE meta_key REGEXP '^_oembed_[0-9a-f]{32}$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we define these regular expressions as class-level constants, and link to where they're defined in core?
0-9a-f]{32} seems like a magical value, so it'd be good to document where it comes from.
| # Generate some various oEmbed caches types | ||
| When I run `wp post generate --post_type=oembed_cache --post_date="2000-01-01" --post_title=$(wp eval 'echo md5( "foo" );') --count=10` | ||
| And I run `wp post meta add 1 _oembed_$(wp eval 'echo md5( "foo" );') foo` | ||
| And I run `wp post meta add 1 _oembed_$(wp eval 'echo md5( "bar" );') bar` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use WordPress APIs to generate real oEmbed cache values?
If we do so, these will be true integration tests and can alert us to breakage if core ever changes its behavior.
Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>
Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>
Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>
When clearing post meta caches, WordPress deletes every meta key starting with In the current state of core oembed (and it's been like this for a while), it's actually impossible that However, WordPress will still consider I'm trying here to be more accurate by using the REGEX query and only target real oembeds keys (although it's quite unlikely So the question is: do we stick the WordPress core behavior (e.g. delete all keys starting with I have no strong opinion on this, both are fine. |
I'm fine with this more precise behavior. Is there a Trac ticket to make Core more precise too? |
| * | ||
| * # Clear cache for a post | ||
| * $ wp embed cache clear 123 | ||
| * Success: Cleared oEmbed cache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change the message to include the post ID.
| * Success: Cleared oEmbed cache. | |
| * Success: Cleared oEmbed cache for post 123. |
|
I honestly think it's pretty silly to go this precise with it. If a 3rd party adds to the cache in a non-standard way. They are probably happy about it being deleted when the user intends to delete it. There is a prefix that is specifically for oEmbed caches, a post type literally named Anyway, thanks for the code, I am using it in my plugin. Without the pattern checking, even though I do not write anything custom to it. What would be the use case for writing something custom to it and then expect it to be excluded from deletions? |
wp embed cache clear --all command (#67)wp embed cache clear --all command
oembed_cache), post metas (_oembed_{hash},_oembed_time_{hash}), transients (_transient_oembed_{hash},_transient_oembed_time_{hash}) cachesFixes #67