Fix: Bug Inconsistency: Standardize post meta key naming with the wpai_ prefix - #867
Fix: Bug Inconsistency: Standardize post meta key naming with the wpai_ prefix#867hbhalodia wants to merge 7 commits into
wpai_ prefix#867Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #867 +/- ##
=============================================
+ Coverage 80.43% 80.49% +0.05%
- Complexity 2565 2568 +3
=============================================
Files 110 111 +1
Lines 10448 10482 +34
=============================================
+ Hits 8404 8437 +33
- Misses 2044 2045 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
dkotter
left a comment
There was a problem hiding this comment.
Left a few comments but overall this looks good. I do see there's still references to the old meta keys in some of our documentation so also need another sweep across the entire plugin to ensure all references are updated
| $this->rename_post_meta_key( 'ai_generated', 'wpai_generated' ); | ||
| $this->rename_post_meta_key( 'ai_generated_summary', 'wpai_generated_summary' ); | ||
| $this->rename_comment_meta_key( 'ai_note', 'wpai_note' ); |
There was a problem hiding this comment.
Since we're using direct DB queries to update meta, may need a cache flush after this runs in order to ensure any cached values are flushed out. Likely not a problem as we're not changing data, just keys but may be better safe than sorry
There was a problem hiding this comment.
Sure, Will flush the cache to ensure everything works expected.
There was a problem hiding this comment.
wp_cache_flush works and is likely fine since this is a one-time thing but does clear out the entire cache, whereas we only need to worry about the post and comment cache. We could modify things to track which post IDs and comment IDs were updated and then just clear those caches (potentially using clean_post_cache/clean_comment_cache). Any thoughts on that or is that not needed?
There was a problem hiding this comment.
I do not think, clean_post_cache/clean_comment_cache is needed, as wp_cache_flush would wipe every cache on the site. So IMO it's not needed as post/comment and every object cache would be flushed.
There was a problem hiding this comment.
Yes, wp_cache_flush will clear the cache we need here but also deletes all other caches. So on sites heavily reliant on cached data for performance, running wp_cache_flush can have a negative impact on the site. Likely won't be a problem here but we could be more specific with the cache flush functions to only clear what we need
There was a problem hiding this comment.
Thanks, also this is a one off thing, so we can go with wp_cache_flush here.
This is now done in commit - 2d53db7 Thanks, |
| register_meta( | ||
| 'comment', | ||
| 'ai_note', | ||
| 'wpai_note', |
There was a problem hiding this comment.
Something I hadn't thought about until now, these meta keys we're renaming are set as show_in_rest => true. This means this data is exposed via REST and there could be others that are consuming/using that data. This rename would break their integrations.
I would guess there's a low (potentially zero) amount of users doing this but any thoughts on what is a breaking change here? At the very least, we'll need to call this out as a breaking change in our changelog but wondering if there's anything else we should consider here
There was a problem hiding this comment.
I would guess there's a low (potentially zero) amount of users doing this but any thoughts on what is a breaking change here?
Yes, there could be breaking change, for eg; Users using/depend on Rest API for data would break, since ai_generated key would no longer be return in response, it would be wpai_generated. So user who access it as meta.ai_generated would get undefined, so it would silently break that part.
Another example could be user external writes request with body params as {"meta": {"ai_generated": ""}} would break, because rest does not knows about this key, so it would silently drops this key. So until noticed it won't be visible.
May be we have 2 pathways here,
- Document it as a breaking change in newer version. Simplest but may have backward compatibility issue. But since amount of users would potentially be zero doing this would work.
- Keep the meta key and register newer one side by side, drop the other one after few releases and add the deprecation notice or similar using the older keys could be an option.
IMO, we should go with the 1st, just document it as a breaking change in the release doc.
There was a problem hiding this comment.
Yeah, I think I'm fine with option 1 here. But probably worth us discussing @jeffpaul prior to merging this in
There was a problem hiding this comment.
I've not had a chance to discuss with Jeff but I'm still fine with option 1 here so let's move forward with that
Edit: just saw Jeff left a comment on the Issue. I'm fine with the renaming
There was a problem hiding this comment.
Thanks, @dkotter I think this would be added at the time of release right? So no any changes in PR.
Let me know if there is any other feedback on this?
What?
Closes #866
Why?
How?
Use of AI Tools
Testing Instructions
Screenshots or screencast
Changelog Entry