Make WordPress Core

Opened 12 months ago

Last modified 5 days ago

#64156 new defect (bug)

add_post_type_support() does not merge sub-properties for the same feature

Reported by: fabiankaegy Owned by:
Priority: normal Milestone: 7.2
Component: Posts, Post Types Version:
Severity: normal Keywords: has-patch has-unit-tests 2nd-opinion early needs-refresh changes-requested
Cc: Focuses:

Description (last modified by fabiankaegy)

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:

  1. Block editor settings - Different plugins adding different editor configuration options
  2. Custom feature flags - Themes and plugins progressively enhancing the same feature
  3. 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.

Attachments (1)

64156.diff​ (14.2 KB ) - added by codesawmir 3 weeks ago.
Refreshed patch addressing the unit test feedback on PR 10426, with the @since bumped to 7.2.0.

Download all attachments as: .zip

Change History (38)

This ticket was mentioned in ​Slack in #core-editor by fabiankaegy. ​View the logs.


12 months ago

This ticket was mentioned in ​PR #10426 on ​WordPress/wordpress-develop by ​@fabiankaegy.


12 months ago
#2

  • Keywords has-patch has-unit-tests added

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

#3 @fabiankaegy
12 months 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.

#4 @fabiankaegy
12 months ago

  • Description modified (diff)

​@Mamaduka commented on ​PR #10426:


12 months ago
#5

IMO, the current behavior is broken, but given everything WP, would this be considered a breaking change?

cc @desrosj, @peterwilsoncc

#6 @joemcgill
12 months 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.

#7 @wildworks
12 months ago

  • Milestone Awaiting Review → 6.9

#8 @wildworks
12 months ago

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,
) );

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 @tusharaddweb
11 months 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 @mindctrl
11 months 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 @wildworks
11 months 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 @wildworks
11 months 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 @fabiankaegy
11 months 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 @wildworks
11 months 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 @fabiankaegy
11 months 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:


11 months ago
#16

The RC1 release is approaching, so we need to decide whether or not to ship this pull request. What do you think?

#17 @mindctrl
11 months ago

  • Keywords 2nd-opinion added

​@wildworks commented on ​PR #10426:


11 months 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:


11 months ago
#19

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?

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:


11 months 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:


11 months 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:


11 months 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 @swissspidy
11 months ago

  • Component General → 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 @wildworks
11 months 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 @davidbaumwald
11 months ago

  • Milestone 6.9 → 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.

This ticket was mentioned in ​Slack in #core-test by mohkatz. ​View the logs.


8 months ago

#27 @ozgursar
8 months ago

Patch Testing Report

Patch Tested: ​https://github.com/WordPress/wordpress-develop/pull/10426

Environment

  • WordPress: 7.0-alpha-61215-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 144.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.4
  • MU Plugins: None activated
  • Plugins:
    • Code Snippets 3.9.5
    • Test Reports 1.2.1

Steps taken

  1. Add the plugin below via Code Snippets or functions.php to display the results of add_post_type_support with or without patch
  2. View the Dashboard and note the results in the Merge Bug Testwidget
  3. ✅ Patch is solving the problem

Expected result

  • After applying the patch both properties are properly merged into the array.

Screenshots/Screencast with results

Before:
https://i.imgur.com/QxQW7aJ.png

After:
https://i.imgur.com/Ealz9hi.png

Support Content

Code snippet for the dashboard widget created with the help of Claude.ai

add_action('wp_dashboard_setup', 'test_post_type_support_bug');

function test_post_type_support_bug() {
    wp_add_dashboard_widget(
        'post_type_support_test',
        'add_post_type_support() Merge Bug Test',
        'display_post_type_support_test'
    );
}

function display_post_type_support_test() {
    // Clear any existing support first
    remove_post_type_support('page', 'editor');
    
    // First call - add 'default-mode' property
    add_post_type_support('page', 'editor', array(
        'default-mode' => 'template-locked',
    ));
    $result1 = get_all_post_type_supports('page')['editor'];
    
    // Second call - add 'block-comments' property
    add_post_type_support('page', 'editor', array(
        'block-comments' => true,
    ));
    $result2 = get_all_post_type_supports('page')['editor'];
    
    echo '<div style="font-family: monospace;">';
    echo '<h4>After first call:</h4>';
    echo '<pre style="background: #f0f0f0; padding: 10px;">';
    print_r($result1);
    echo '</pre>';
    
    echo '<h4>After second call:</h4>';
    echo '<pre style="background: #ffe6e6; padding: 10px;">';
    print_r($result2);
    echo '</pre>';
    
    echo '<h4>Expected both properties:</h4>';
    echo '<pre style="background: #e6f3ff; padding: 10px;">';
    echo "Array\n(\n    [0] => Array\n        (\n            [default-mode] => template-locked\n            [block-comments] => 1\n        )\n)\n";
    echo '</pre>';
    echo '</div>';
}

#28 @audrasjb
7 months ago

  • Keywords early added
  • Milestone 7.0 → 7.1

As per today's 7.0 pre-RC1 bug scrub:

I concur with David's last comment during 6.9 cycle:

This is early-adjacent given the impact of changes to add_post_type_support and surface area.

7.0 RC1 is tomorrow. Let's move it to 7.1 with early keyword.

This ticket was mentioned in ​Slack in #core by jorbin. ​View the logs.


7 months ago

#30 @wildworks
3 months ago

  • Keywords needs-refresh added
  • Milestone 7.1 → 7.2

To move this ticket forward, we need to address the feedback on ​PR 10426. However, that PR changes the behavior of add_post_type_support(), so we cannot commit it at this stage. I'd like to punt it to 7.2 anyway.

This ticket was mentioned in ​Slack in #core by audrasjb. ​View the logs.


4 weeks ago

#32 @audrasjb
4 weeks ago

  • Keywords changes-requested added

As per today's 7.2 bug scrub:

The PR is on it's way towards a resolution, but we still needs some unit tests changes.
@fabiankaegy do you think you'll have time to address the feedback in the PR?
Otherwise, anyone is welcome to create a new PR to address the changes suggested in unit tests.
Same as other early tickets, this one will get punted early October.

@codesawmir
3 weeks ago

Refreshed patch addressing the unit test feedback on PR 10426, with the @since bumped to 7.2.0.

#33 @codesawmir
3 weeks ago

Refreshed patch in attachment:64156.diff​, addressing the feedback on PR 10426:

  • @since updated to 7.2.0 (per @audrasjb).
  • Test cleanup moved into a tear_down() method, so it is not skipped when an assertion fails, and custom assertion messages added to the tests (per @swissspidy).
  • Tests added for merging arguments that are not associative arrays (per @westonruter's TODO and @Mamaduka's post-formats example): appending a list, adding string keys to an existing list, and merging an empty array.

One change to the merge itself: it now assigns through $args before storing, instead of writing to the stored array and returning early. Previously the arguments after the first one were never updated, so a leftover extra argument survived a later call that never passed one, and an extra argument passed to a later call was dropped. Only the first argument is mergeable, so the rest now behave as they did before this ticket: replaced by the most recent call.

Not introduced here, but worth a decision before this lands: WP_Post_Type::add_supports() is the only caller in core that passes array arguments, and it does not clear $_wp_post_type_features first, so re-registering a post type inherits arguments the new registration never declared:

<?php
register_post_type( 'probe', array( 'supports' => array( 'editor' => array( 'default-mode' => 'template-locked' ) ) ) );
register_post_type( 'probe', array( 'supports' => array( 'editor' => array( 'block-comments' => true ) ) ) );

get_all_post_type_supports( 'probe' )['editor'];
// before: array( array( 'block-comments' => true ) )
// after:  array( array( 'default-mode' => 'template-locked', 'block-comments' => true ) )

Clearing the features there would restore the previous result, but would also drop features added through separate add_post_type_support() calls between registrations. Happy to follow whichever direction is preferred, here or in a follow-up.

Tested on PHP 8.3, single site: tests/phpunit/tests/post/types.php (59 tests, 186 assertions) and --group post (962 tests, 2737 assertions) pass, and phpcs reports no errors on either changed file.

​@mindctrl commented on ​PR #10426:


12 days ago
#34

@fabiankaegy do you have the bandwidth to address the review feedback? There's a patch attached to the Trac ticket that addresses all/most of it, but it's nice to have it here on GitHub where the test suite runs. I can open another PR if you don't have the time.

#35 @fabiankaegy
11 days ago

Refreshed ​PR 10426 based on @codesawmir's patch in attachment:64156.diff​. Thanks for picking this up!

Two changes on top of the patch:

  • Dropped the new tear_down(). The base test case already unregisters post types in set_up() via reset_post_types(), so the _unregister_post_type() calls can simply go, as @peterwilsoncc pointed out.
  • Fixed a leak in the merge. The patch writes the merged result back into $args[0], but $args is shared across the loop over $features. So add_post_type_support( 'foo', array( 'a', 'b' ), array( 'shared' => true ) ) copied feature a's existing arguments into b. It now merges into a copy per feature, with a test.

I also clarified the docblock, since only the first argument gets merged and any further ones are replaced by the latest call.

The two follow-ups that came up in the review now have their own tickets, each with a PR stacked on top of this one:

  • #66224 / ​PR 13884: remove_post_type_support( 'custom', 'editor', 'notes' ) removes a single sub-feature and keeps the rest (raised by @wildworks).
  • #66225 / ​PR 13885: post_type_supports( 'custom', 'editor', 'notes' ) checks for a single sub-feature (suggested by @mamaduka).

​@peterwilsoncc commented on ​PR #10426:


7 days ago
#36

IMO, the current behavior is broken, but given everything WP, would this be considered a breaking change?

cc @desrosj, @peterwilsoncc

This is a tough one... I agree that the current behaviour is very weird (if not broken) but am unsure if changing it will cause problems. Would it be best to introduce a function append_post_type_support or similar? The use of ...$args prevents introducing a new parameter for the purpose.

#37 @audrasjb
5 days ago

Hello, this ticket is marked early so it would be nice to have it committed this week as we are only two weeks from 7.2 beta 1. If it doesn't look realistic, we should move it to Future Release.

The last comment from @peterwilsoncc in the PR proposes another approach, so I'm inclined to say that maybe we should take time to discuss this a bit more…

Note: See TracTickets for help on using tickets.