#62997 Remove support for shared terms #8371
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
@johnbillion This mostly looks good to me, and I'm in favor of this cleanup. Just a few points of feedback.
src/wp-includes/taxonomy.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Splits a batch of shared taxonomy terms. | ||
| * | ||
| * @since 4.3.0 | ||
| * | ||
| * @global wpdb $wpdb WordPress database abstraction object. | ||
| */ | ||
| function _wp_batch_split_terms() { |
There was a problem hiding this comment.
Why keep these functions around as empty? (This feedback applies to all similarly treated functions in this file.)
If this is about backward compatibility, I'd argue it's not worth it here for a few reasons:
- This PR already breaks backward compatibility and (rightfully) prevents updates in case the site still needs the functionality.
- These functions are marked private, so they should never be used externally in the first place.
At the very minimum, if we keep them, I think they should be marked deprecated and moved to deprecated.php. But per the above, I think it's really not worth keeping them. We should have a dev note for this change, and there we could also mention that all the private helper functions are being removed.
src/wp-includes/taxonomy.php
Outdated
| @@ -4572,14 +4299,7 @@ function _wp_check_split_nav_menu_terms( $term_id, $new_term_id, $term_taxonomy_ | |||
| * @return array Array of new term IDs, keyed by taxonomy. | |||
| */ | |||
| function wp_get_split_terms( $old_term_id ) { | |||
There was a problem hiding this comment.
These functions are not marked private, so unlike the underscore prefixed ones above, I think these ones it would make sense to formally mark deprecated and move to deprecated.php.
There was a problem hiding this comment.
I agree, I hadn't made a proper decision on these so just emptied them out. I think a formal deprecation makes sense.
| * @return WP_Term|WP_Error|false Term object, if found. WP_Error if `$term_id` is shared between taxonomies and | ||
| * there's insufficient data to distinguish which term is intended. | ||
| * False for other failures. | ||
| * @param string $taxonomy Optional. Limit matched terms to those matching `$taxonomy`. |
There was a problem hiding this comment.
Maybe clarify:
| * @param string $taxonomy Optional. Limit matched terms to those matching `$taxonomy`. | |
| * @param string $taxonomy Optional. No longer used and silently deprecated. |
There was a problem hiding this comment.
The behaviour of this parameter actually still applies, the existing test_should_return_null_when_provided_taxonomy_does_not_match_actual_term_taxonomy() test covers it. It prevents a term in a different category being returned.
| include: | ||
| - os: 'ubuntu-24.04' | ||
| php: '7.2' | ||
| db-type: 'mysql' | ||
| db-version: '5.7' | ||
| wp: 4.2 | ||
| multisite: false | ||
| - os: 'ubuntu-24.04' | ||
| php: '7.2' | ||
| db-type: 'mysql' | ||
| db-version: '5.7' | ||
| wp: 4.7 | ||
| multisite: false | ||
| - os: 'ubuntu-24.04' | ||
| php: '7.2' | ||
| db-type: 'mysql' | ||
| db-version: '5.7' | ||
| wp: 5.2 | ||
| multisite: false | ||
| - os: 'ubuntu-24.04' | ||
| php: '7.2' | ||
| db-type: 'mysql' | ||
| db-version: '5.7' | ||
| wp: 5.7 | ||
| multisite: false | ||
| - os: 'ubuntu-24.04' | ||
| php: '7.2' | ||
| db-type: 'mysql' | ||
| db-version: '5.7' | ||
| wp: 6.2 | ||
| multisite: false |
There was a problem hiding this comment.
Why are all of these being removed as part of this PR?
There was a problem hiding this comment.
I guess I should have made it clear that I'm still working on this PR and it's not quite ready for review yet (but thanks anyway!). I added these matrix entries to debug an issue I was seeing when experimenting with testing an upgrade from 4.2 right to 6.8. I'll remove or adjust these once I'm done.
Trac ticket: https://core.trac.wordpress.org/ticket/62997