Migrate EM to use term_meta #41
Conversation
| add_action( 'enqueue_block_editor_assets', [ __CLASS__, 'load_styles_for_block_editor' ] ); | ||
|
|
||
| // Add metadata fields to term | ||
| add_filter( 'get_term', [ __CLASS__, 'add_metadata_to_term' ], 10, 1 ); |
There was a problem hiding this comment.
Both of these allow us to massage the terms to add metadata info in, and save us an extra call when doing gets.
| $field = 'term_id'; | ||
| $term = get_term( $value, self::METADATA_TAXONOMY ); | ||
| } else { | ||
| $term = get_term_by( $field, $value, self::METADATA_TAXONOMY ); |
There was a problem hiding this comment.
Made this get_by way more efficient
| * @return WP_Term $term The term with metadata added | ||
| */ | ||
| public static function add_metadata_to_term( WP_Term $term ): WP_Term { | ||
| if ( ! isset( $term->taxonomy ) || self::METADATA_TAXONOMY !== $term->taxonomy ) { |
There was a problem hiding this comment.
Added in some quick exits so this gets done faster
| return $type_meta_result; | ||
| } else if ( ! $type_meta_result ) { | ||
| // If we can't save the type, we should delete the term | ||
| wp_delete_term( $term_id, self::METADATA_TAXONOMY ); |
There was a problem hiding this comment.
Added in some cleanup code in case of a failure. My whole site failed when this happened so this is critical imo
| delete_post_meta_by_key( $post_meta_key ); | ||
|
|
||
| $result = wp_delete_term( $term_id, self::METADATA_TAXONOMY ); | ||
| delete_term_meta( $term_id, self::METADATA_TYPE_KEY ); |
There was a problem hiding this comment.
Didn't bother with the result as the core term is being killed. This stopped deletes when it shouldn't have
| } | ||
| return eMetadataTerm; | ||
| } ) | ||
| .sort( ( termA, termB ) => termA.name.localeCompare( termB.name ) ) |
|
|
||
| foreach ( $editorial_metadata_terms as $term ) { | ||
| $post_meta_key = self::get_postmeta_key( $term ); | ||
| $post_meta_key = $term->meta[ self::METADATA_POSTMETA_KEY ]; |
There was a problem hiding this comment.
I think separating actual WP_Term data from supplied ->meta is a good design choice. Currently custom statuses add all sorts of extras to the WP_Term directly, which can make it hard to tell what's built-in and what's dynamically added. We should probably do the same for custom statuses in the future.
There was a problem hiding this comment.
Yip, that was my intention with this. I didn't like how we were stuffing so much extra info in the description, when really that should have been going in as metadata.
alecgeatches
left a comment
alecgeatches
left a comment
There was a problem hiding this comment.
Left a handful of comments below. Overall, I like the design choices you've made to store meta separately and most of the implementation. There are a couple of changes I'd like to see before merging:
-
Can we figure out why multisite tests are failing? It's nice to have these running to detect actual multisite issues, and I don't want to turn these off and miss out on that information.
-
For some reason, I'm not seeing date as a supported type on the EM page:
Any idea why that's happening?
|
Thanks for fixing the multisite test problem! As for the date not showing up as a valid type, I checked and I'm not able to replicate that. I tried on a fresh and existing install. Maybe it's a weird caching issue? |
Description
This PR migrates the storage system used by Editorial Metadata to be
term_metafor the metadata items liketypeandpostmeta key. This will set the stage for the same system to be used in custom status as well.For now, the position ordering has been removed in favour of name based sorting. This is to keep things simple when it comes to the front end code. This will be needed for the custom status migration anyways, so we can duplicate that code then or when we add drag/drop support. This way, at least there's some order to the whole system.
I've also disabled the multisite tests as it was randomly causing 401 problems for the rest api. I've marked why we have disabled this, and this at least allows EM tests to be written. It seems like something to do with the user but we can check this out later.
Steps to Test