Opened 6 weeks ago
Last modified 4 weeks ago
#64156 new defect (bug)
add_post_type_support() does not merge sub-properties for the same feature
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | 7.0 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Posts, Post Types | Keywords: | has-patch has-unit-tests 2nd-opinion |
| Focuses: | Cc: |
Description (last modified by )
Summary
When add_post_type_support() is called multiple times for the same post type and feature with associative array arguments, the second call overwrites the first call's arguments instead of merging them. This prevents incremental addition of sub-properties to post type features.
Current Behavior
add_post_type_support( 'page', 'editor', array(
'default-mode' => 'template-locked',
) );
add_post_type_support( 'page', 'editor', array(
'block-comments' => true,
) );
// Result: Only 'block-comments' is stored, 'default-mode' is lost
var_dump( get_all_post_type_supports( 'page' )['editor'] );
// array( array( 'block-comments' => true ) )
Expected Behavior
Multiple calls should merge the array arguments, allowing different plugins/themes to add their own sub-properties without conflicting:
add_post_type_support( 'page', 'editor', array(
'default-mode' => 'template-locked',
) );
add_post_type_support( 'page', 'editor', array(
'block-comments' => true,
) );
// Expected: Both properties should be preserved
var_dump( get_all_post_type_supports( 'page' )['editor'] );
// array( array( 'default-mode' => 'template-locked', 'block-comments' => true ) )
Technical Details
The issue is in /src/wp-includes/post.php at line 2292, where the function simply overwrites the existing value:
if ( $args ) {
$_wp_post_type_features[ $post_type ][ $feature ] = $args;
}
Proposed Solution
Detect when both the existing and new values are associative arrays and merge them using array_merge(). This maintains backwards compatibility because:
- If the feature doesn't exist yet, it's created as before
- If the feature is set to
true(no args), calling with args still overwrites it as expected - Only when both values are arrays does merging occur
- Non-associative array arguments continue to work as before
Use Cases
This is particularly useful for:
- Block editor settings - Different plugins adding different editor configuration options
- Custom feature flags - Themes and plugins progressively enhancing the same feature
- Multi-plugin compatibility - Avoiding conflicts when multiple plugins extend the same post type feature
Impact
This change would improve the API's usability and prevent unexpected behavior when multiple plugins/themes interact with the same post type features. The fix maintains full backwards compatibility.
Change History (25)
This ticket was mentioned in Slack in #core-editor by fabiankaegy. View the logs.
6 weeks ago
This ticket was mentioned in PR #10426 on WordPress/wordpress-develop by @fabiankaegy.
6 weeks ago
#2
- Keywords has-patch has-unit-tests added
#3
@
6 weeks ago
The reason why this became an issue now and why I would suggest we fix this in time for WordPress 6.9 is that in 6.7 we shipped the editor post type support sub-key
default-mode. And now in 6.9 we are adding the notes sub-key.
So when there is existing code out there that adds the default-mode it now causes the notes support to be overwritten.
@Mamaduka commented on PR #10426:
6 weeks ago
#5
IMO, the current behavior is broken, but given everything WP, would this be considered a breaking change?
cc @desrosj, @peterwilsoncc
#6
@
6 weeks ago
@fabiankaegy asked for my opinion on this issue, and I think it would be totally fine to enhance the current implementation of add_post_type_support() so that multiple calls for the same post type and feature with different args would merge the args rather than overwrite them.
However, I think before doing so, we should consider whether overloading the args of the editor feature like this is the best choice.
It's important to remember that add_post_type_support() and the related subsystem was originally added in #9674 in order to help signal which features should be supported in the editor for custom post types. Quoting the dev resources site for this function:
All core features are directly associated with a functional area of the edit screen, such as the editor or a meta box. Features include: ‘title’, ‘editor’, ‘comments’, ‘revisions’, ‘trackbacks’, ‘author’, ‘excerpt’, ‘page-attributes’, ‘thumbnail’, ‘custom-fields’, and ‘post-formats’.
Rather than adding more args to the editor feature, it's probably better to introduce new features to this system, like:
add_theme_support( 'page', 'block-comments' );
Rather than overloading the args, like:
add_post_type_support( 'page', 'editor', array(
'block-comments' => true,
) );
Juggling several unrelated sets of args for the same feature seems like it will be more fragile and lead to more conflicts between plugins and themes trying to customize unrelated args for a single feature.
#8
@
6 weeks ago
Rather than adding more args to the
editorfeature, it's probably better to introduce new features to this system, like:
add_theme_support( 'page', 'block-comments' );Rather than overloading the args, like:
add_post_type_support( 'page', 'editor', array( 'block-comments' => true, ) );
This was discussed previously. The reason Notes was made a sub-feature is that it cannot work without an editor. See https://github.com/WordPress/gutenberg/pull/71682#issuecomment-3297216447
#9
@
5 weeks ago
the issue is still there so better to patch in core as under wp-includes/post.php
replace the add_post_type_support function code with below code so it will remove the problems
function add_post_type_support( $post_type, $feature, $args = true ) {
global $_wp_post_type_features;
if ( ! isset( $_wp_post_type_features[ $post_type ] ) ) {
$_wp_post_type_features[ $post_type ] = [];
}
if ( is_array( $feature ) ) {
foreach ( $feature as $f ) {
add_post_type_support( $post_type, $f, $args );
}
return;
}
if ( isset( $_wp_post_type_features[ $post_type ][ $feature ] )
&& is_array( $_wp_post_type_features[ $post_type ][ $feature ] )
&& is_array( $args )
) {
$_wp_post_type_features[ $post_type ][ $feature ] = array_merge(
$_wp_post_type_features[ $post_type ][ $feature ],
$args
);
} else {
$_wp_post_type_features[ $post_type ][ $feature ] = $args;
}
}
#10
@
5 weeks ago
The attached PR is in good shape with great test coverage. There is a remaining question about back compat. I'd personally like to see this land. Given the function name is add_post_type_support(), I would not expect that someone adding an additional feature after I've added mine would result in mine being wiped out.
#11
@
5 weeks ago
@tusharaddweb When reporting the test results, it's a good idea to actually apply the patch and see if it resolves the problem.
See: https://make.wordpress.org/core/handbook/tutorials/working-with-patches/
#12
@
5 weeks ago
Personally, I think modifying the behavior of this function in 6.9 release might have an impact.
While it's true that subsequent calls overwriting the sub-feature might not be the intended behavior, some consumers might implicitly depend on that behavior. If we decide to merge the sub-functions, sub-functions that were previously hidden by the overwrite might become exposed.
For the 6.9 release, I suggest covering this issue in the dev notes, but what do you think?
In other words, we would provide a code snippet like this for users who explicitly want to merge the sub-feature:
https://github.com/WordPress/gutenberg/issues/66377#issuecomment-3480056848
What do you think?
#13
@
5 weeks ago
My problem with this approach is that we specifically advertised the post type support editor sub property in WordPress 6.7 for the default rendering mode. So there is code out there in the wild where 3rd parties are using that today.
By releasing 6.9 as it works today (even if we annotate it in the dev note) those sites will not have notes support on those post types. So without having done anything wrong they don't get a prominently highlighted feature of the release unless they update their code that we told them to include before.
However I do also see that we are likely just too late in the 6.9 cycle to safely land this...
#14
@
5 weeks ago
By releasing 6.9 as it works today (even if we annotate it in the dev note) those sites will not have notes support on those post types.
This is indeed a problem. Users who change the default rendering mode of the page might report a bug because they can't use Nnotes on the page...
#15
@
5 weeks ago
Yeah that is what happened to me in Beta one which started off this whole thread 😂
I have several client sites that leverage this theme support out in the wild already.
@wildworks commented on PR #10426:
5 weeks ago
#16
The RC1 release is approaching, so we need to decide whether or not to ship this pull request. What do you think?
@wildworks commented on PR #10426:
4 weeks ago
#18
This pull request seems very robust, but my only concern is that it behaves differently from remove_post_type_support(). Currently, the remove_post_type_support() function shouldn't be able to remove only sub-features. What do you think?
@westonruter commented on PR #10426:
4 weeks ago
#19
This pull request seems very robust, but my only concern is that it behaves differently from
remove_post_type_support(). Currently, theremove_post_type_support()function shouldn't be able to remove only sub-features. What do you think?
For completeness it seems that being able to remove sub-features via remove_post_type_support() makes sense. I don't know how common of a need it is, however. So this would entail adding a new $args param to remove_post_type_support() to match add_post_type_support():
function add_post_type_support( $post_type, $feature, ...$args )
So:
function remove_post_type_support( $post_type, $feature, ...$args )
But how would this be used? If $args[0] is an associative array, would it remove the supplied keys only if the values match? That would be the case if using array_diff(). But what if the user wants to remove the sub-feature regardless of the value?
@wildworks commented on PR #10426:
4 weeks ago
#20
For completeness it seems that being able to remove sub-features via
remove_post_type_support()makes sense. I don't know how common of a need it is,
I believe this need exists because the editor feature now has multiple sub-futures.
Consider a scenario where multiple sub-functions are registered as shown below, and you want to disable only the 'notes' sub-feature:
add_post_type_support( 'custom', 'editor', array(
'default-mode' => 'template-locked',
'notes' => true,
) );
You will need to remove the existing main feature and then register the necessary sub-feature again, as follows.
remove_post_type_support( 'custom', 'editor' );
add_post_type_support( 'custom', 'editor', array(
'default-mode' => 'template-locked',
) );
I don't think this concern will be a blocker here, but I wanted to share it just in case.
@johnjamesjacoby commented on PR #10426:
4 weeks ago
#21
Consider also that $feature could be an array and $args gets set to all of them...
add_post_type_support( 'post', array( 'title', 'author' ), array( 'jjj' => 'cool' ) );
...and that remove_post_type_support() only supports removing a single $feature, so there already is not parity between them.
---
I think using $args as sub-features was not a great idea for the reasons highlighted by the discussion here, and that every sub-feature should be a $feature with a unique key with its own (or no) arguments.
Is it too late to create a new feature key instead?
@Mamaduka commented on PR #10426:
4 weeks ago
#22
Consider also that $feature could be an array and $args gets set to all of them
I think this was/is an "issue" even without this patch.
remove_post_type_support() only supports removing a single $feature, so there already is not parity between them.
Agreed, it makes sense to have feature parity here. We should also update post_type_supports and allow checking sub-features.
I think using $args as sub-features was not a great idea for the reasons highlighted by the discussion here, and that every sub-feature should be a $feature with a unique key with its own (or no) arguments.
Initially, it was implemented this way, but we changed it later, since these are the actual sub-features for editor. Notes won't work without editor support.
cc @swissspidy
#23
@
4 weeks ago
- Component changed from General to Posts, Post Types
I'd consider this an enhancement and as such should be dealt with in 7.0, especially since we're about to release RC1.
#24
@
4 weeks ago
I'd consider this an enhancement and as such should be dealt with in 7.0, especially since we're about to release RC1.
I tend to agree with this. The concern, as already pointed out, is that the Notes feature will become unavailable if consumers opt into the editor's sub-features as described below:
// This code unintentionally removes the Note feature from the Page.
add_post_type_support( 'page', 'editor', array(
'default-mode' => 'template-locked',
) );
This could perhaps be addressed with developer notes, as suggested here: https://github.com/WordPress/gutenberg/issues/66377#issuecomment-3480056848
#25
@
4 weeks ago
- Milestone changed from 6.9 to 7.0
Agreed. This is early-adjacent given the impact of changes to add_post_type_support and surface area.
With open questions remaining and 6.9 RC1 releasing shortly, moving this
to 7.0.
If any committer feels the remaining issues can be resolved in time for 6.9 RC1 and wish to assume ownership, feel free to update the milestone accordingly.
Previously, calling
add_post_type_support()multiple times for the same post type and feature with array arguments would overwrite the previous arguments instead of merging them. This prevented plugins and themes from incrementally adding sub-properties to the same feature.This changes the behavior to merge associative array arguments when the function is called multiple times with the same post type and feature combination.
Example usage:
add_post_type_support( 'page', 'editor', array( 'default-mode' => 'template-locked', ) ); add_post_type_support( 'page', 'editor', array( 'block-comments' => true, ) ); // Both properties are now preserved instead of only the last one.The fix maintains full backwards compatibility by only merging when both the existing and new values are associative arrays. All other calling patterns continue to work as before.
Props @fabiankaegy, @Mamaduka, @t-hamano .
Trac ticket: https://core.trac.wordpress.org/ticket/64156#ticket