Add support for `wp embed cache clear --all` command (#67) #74

nlemoine · 2023-09-01T08:55:44Z

Removes posts (oembed_cache), post metas (_oembed_{hash}, _oembed_time_{hash}), transients (_transient_oembed_{hash}, _transient_oembed_time_{hash}) caches
Add tests

Fixes #67

- Removes posts (`oembed_cache`), post metas (`_oembed_{hash}`, `_oembed_time_{hash}`), transients (`_transient_oembed_{hash}`, `_transient_oembed_time_{hash}`) caches - Add tests

nlemoine · 2023-09-01T09:07:23Z

A couple of notes on this PR:

I went for a REGEXP MySQL query when deleting all caches.
- This will be significantly faster than deleting caches using WP methods (delete_post_meta, etc.). Downside is it does not trigger delete_{type} hook (delete_{$meta_type}_metadata, etc.). I don't know if this is considered a problem.
- This ensures the query only targets oembed caches. Although it's very unlikely, some key named _oembed_foo will not be deleted.
The testing workflow will fail on WP 3.7. I don't know when oembed_cache post type was introduced and not sure how to handle this in tests (@require-wp-4.9 on the whole scenario event if some parts of it targets versions before this one?). Any guidance appreciated on this topic :)

swissspidy · 2023-09-01T10:49:29Z

I don't know if this is considered a problem.

It's a problem when using an external object cache, because then transients will be in the object cache and not in the database.

nlemoine · 2023-09-01T10:59:41Z

Hello @swissspidy,

Thanks for the prompt reply.

It's a problem when using an external object cache, because then transients will be in the object cache and not in the database.

I'm not sure to get what you meant here 😅

I'm not specifically talking about transients. It's a more global consideration on delete hooks, whether it is for posts, metas or transients.
Object cache seems unrelated here? The delete process in the transient command itself does not care about about object cache.

nlemoine · 2023-09-01T11:03:06Z

The delete process in the transient command itself does not care about about object cache.

Whoops, it actually does but just in a form of a warning message:

https://github.com/wp-cli/cache-command/blob/5a74fa042ecaeff93f149b08a279730c69b1b448/src/Transient_Command.php#L610-L612

nlemoine · 2023-09-20T11:47:17Z

@swissspidy @danielbachhuber friendly bump 🙂

Anything I can do to help?

danielbachhuber

@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

nlemoine · 2023-09-27T19:44:09Z

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 _oembed_{md5_hash}? Although it's more accurate, WordPress is considering any string starting with _oembed as an oembed cache:

https://github.com/WordPress/WordPress/blob/565c2179ffefe99293f73d936388aac52ce2e878/wp-includes/class-wp-embed.php#L393

embed-command/features/cache.feature

Lines 18 to 31 in 3987e20

    
               When I run `wp post-meta add 1 _oembed_foo 'bar'` 
        
               Then STDOUT should not be empty 
        
               When I run `wp post-meta get 1 _oembed_foo` 
        
               Then STDOUT should be: 
        
                 """ 
        
                 bar 
        
                 """ 
        
               When I run `wp embed cache clear 1` 
        
               Then STDOUT should be: 
        
                 """ 
        
                 Success: Cleared oEmbed cache. 
        
                 """

Should I also introduce a wp embed cache trigger --all command to warm all oembed caches?

danielbachhuber

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 _oembed as an oembed cache:

Could you share more details about what you're thinking about?

Should I also introduce a wp embed cache trigger --all command to warm all oembed caches?

Could! Feel free to open a separate GitHub issue and we can discuss how it might work.

danielbachhuber · 2023-10-02T20:26:05Z

features/cache.feature

+    Then STDOUT should be:
+      """
+      Success: Cleared 3 oEmbed caches: 1 post cache, 1 oembed post cache, 1 transient cache.
+      """


Can we also add some similar-looking meta, and then include verification that it doesn't get unexpectedly deleted?

src/Cache_Command.php

danielbachhuber · 2023-10-02T20:29:22Z

src/Cache_Command.php

+
+				WP_CLI::success( $message );
+			} else {
+				WP_CLI::error( 'No oEmbed caches to clear!' );


Suggested change

WP_CLI::error( 'No oEmbed caches to clear!' );

WP_CLI::error( 'No oEmbed caches to clear.' );

Should this be WP_CLI::success()?

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.

src/Cache_Command.php

danielbachhuber · 2023-10-02T20:30:19Z

src/Cache_Command.php

+			WP_CLI::error( 'You cannot specify both a post id and use --all' );
+		}
+
+		// Delete all oEmbed caches.


This comment is superfluous

danielbachhuber · 2023-10-02T20:37:06Z

src/Cache_Command.php

+			// 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}$'


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.

danielbachhuber · 2023-10-02T20:41:48Z

features/cache.feature

+    # 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`


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 <[email protected]>

nlemoine · 2023-10-04T18:52:24Z

Could you share more details about what you're thinking about?

When clearing post meta caches, WordPress deletes every meta key starting with _embed_ (see links to code in #74 (comment)), whether it's _embed_foo or _oembed_05e38483c61abaf76f8ee2f51139961c.

In the current state of core oembed (and it's been like this for a while), it's actually impossible that _oembed_foo exists. If it does, it has been inserted by third party code.

However, WordPress will still consider _oembed_foo an oembed post meta key and delete it, no question asked.

I'm trying here to be more accurate by using the REGEX query and only target real oembeds keys (although it's quite unlikely _oembed_foo is used by some developer AND is not related to oembed caching but who knows).

So the question is: do we stick the WordPress core behavior (e.g. delete all keys starting with _oembed_, no matter what characters follows) ? Or do we target keys more precisely to scope the deletion on actual oembed cache keys (_oembed_05e38483c61abaf76f8ee2f51139961c, e.g. current PR behavior)?

I have no strong opinion on this, both are fine.

danielbachhuber · 2023-10-04T21:02:48Z

Or do we target keys more precisely to scope the deletion on actual oembed cache keys (_oembed_05e38483c61abaf76f8ee2f51139961c, e.g. current PR behavior)?

I'm fine with this more precise behavior. Is there a Trac ticket to make Core more precise too?

martinkrcho · 2024-04-30T04:37:12Z

src/Cache_Command.php

 	 * : ID of the post to clear the cache for.
 	 *
+	 * [--all]
+	 * : Clear all oEmbed caches.
+	 *
 	 * ## EXAMPLES
 	 *
 	 *     # Clear cache for a post
 	 *     $ wp embed cache clear 123
 	 *     Success: Cleared oEmbed cache.


Let's change the message to include the post ID.

Suggested change

* Success: Cleared oEmbed cache.

* Success: Cleared oEmbed cache for post 123.

nextgenthemes · 2024-04-30T09:24:26Z

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 oembed_cache why you think it's a good idea to go out of your way to not delete something when the ask is to "delete all" is beyond me. Maybe I am missing something, but you mentioned core also does not do it this way, so why would WP-CLI then do it this way?

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?

Add support for wp embed cache clear --all command (wp-cli#67)

c8bebd9

- Removes posts (`oembed_cache`), post metas (`_oembed_{hash}`, `_oembed_time_{hash}`), transients (`_transient_oembed_{hash}`, `_transient_oembed_time_{hash}`) caches - Add tests

nlemoine requested a review from a team as a code owner September 1, 2023 08:55

danielbachhuber requested changes Sep 20, 2023

View reviewed changes

Add support for wp embed cache clear --all command

f653c67

Use WordPress functions to delete various oembed caches

danielbachhuber reviewed Oct 2, 2023

View reviewed changes

nlemoine and others added 3 commits October 4, 2023 20:31

Update src/Cache_Command.php

f930f38

Co-authored-by: Daniel Bachhuber <[email protected]>

Update src/Cache_Command.php

10ec823

Co-authored-by: Daniel Bachhuber <[email protected]>

Update src/Cache_Command.php

c868ec3

Co-authored-by: Daniel Bachhuber <[email protected]>

martinkrcho reviewed Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `wp embed cache clear --all` command (#67) #74

Add support for `wp embed cache clear --all` command (#67) #74

nlemoine commented Sep 1, 2023 •

edited by danielbachhuber

Loading

nlemoine commented Sep 1, 2023 •

edited

Loading

swissspidy commented Sep 1, 2023

nlemoine commented Sep 1, 2023

nlemoine commented Sep 1, 2023

nlemoine commented Sep 20, 2023 •

edited

Loading

danielbachhuber left a comment

nlemoine commented Sep 27, 2023

danielbachhuber left a comment

danielbachhuber Oct 2, 2023

danielbachhuber Oct 2, 2023

nlemoine Oct 4, 2023

danielbachhuber Oct 2, 2023

danielbachhuber Oct 2, 2023

danielbachhuber Oct 2, 2023

nlemoine commented Oct 4, 2023 •

edited

Loading

danielbachhuber commented Oct 4, 2023

martinkrcho Apr 30, 2024

nextgenthemes commented Apr 30, 2024 •

edited

Loading

	WP_CLI::error( 'No oEmbed caches to clear!' );
	WP_CLI::error( 'No oEmbed caches to clear.' );

	* Success: Cleared oEmbed cache.
	* Success: Cleared oEmbed cache for post 123.

Add support for wp embed cache clear --all command (#67) #74

Are you sure you want to change the base?

Add support for wp embed cache clear --all command (#67) #74

Conversation

nlemoine commented Sep 1, 2023 • edited by danielbachhuber Loading

nlemoine commented Sep 1, 2023 • edited Loading

swissspidy commented Sep 1, 2023

nlemoine commented Sep 1, 2023

nlemoine commented Sep 1, 2023

nlemoine commented Sep 20, 2023 • edited Loading

danielbachhuber left a comment

Choose a reason for hiding this comment

nlemoine commented Sep 27, 2023

danielbachhuber left a comment

Choose a reason for hiding this comment

danielbachhuber Oct 2, 2023

Choose a reason for hiding this comment

danielbachhuber Oct 2, 2023

Choose a reason for hiding this comment

nlemoine Oct 4, 2023

Choose a reason for hiding this comment

danielbachhuber Oct 2, 2023

Choose a reason for hiding this comment

danielbachhuber Oct 2, 2023

Choose a reason for hiding this comment

danielbachhuber Oct 2, 2023

Choose a reason for hiding this comment

nlemoine commented Oct 4, 2023 • edited Loading

danielbachhuber commented Oct 4, 2023

martinkrcho Apr 30, 2024

Choose a reason for hiding this comment

nextgenthemes commented Apr 30, 2024 • edited Loading

Add support for `wp embed cache clear --all` command (#67) #74

Add support for `wp embed cache clear --all` command (#67) #74

nlemoine commented Sep 1, 2023 •

edited by danielbachhuber

Loading

nlemoine commented Sep 1, 2023 •

edited

Loading

nlemoine commented Sep 20, 2023 •

edited

Loading

nlemoine commented Oct 4, 2023 •

edited

Loading

nextgenthemes commented Apr 30, 2024 •

edited

Loading