REST API: New terms creation with meta causes PHP Notice

[caercam]

Creating new terms with the REST API works fine, as long as there is no meta attached:

POST https://localhost:8080/wp-json/wp/v2/categories
X-WP-Nonce: 1234567890
Content-Type: application/json
{"name":"A new category","description":"This is a new category.","meta":{"custom_meta":"Custom Meta Value"}}
 -- response --
403 Forbidden

<br />
<b>Notice</b>:  Undefined property: WP_Error::$taxonomy in <b>/var/www/wp47/wp-includes/capabilities.php</b> on line <b>282</b><br />
{"code":"rest_cannot_update","message":"Sorry, you are not allowed to edit the custom_meta custom field.","data":{"key":"custom_meta","status":403}}

For the record, here's what happens as far as I can tell. Probably more detailled than what's really needed, but there's two different issues here so I'd rather be precise.

1. WP_REST_Terms_Controller::create_item() calls WP_REST_Meta_Fields::update_value() with two arguments: $request['meta'], which is what we want, and $request['id'], which at this point is null, because we are creating a new term, not editing an existing one.
2. WP_REST_Meta_Fields::update_value() loops through the registered meta keys and calls WP_REST_Meta_Fields::update_meta_value() or WP_REST_Meta_Fields::update_multi_meta_value().
3. Because WP_REST_Terms_Controller::create_item() casted as integer an ID that does not exist, both meta_value update methods get a $object_id = 0, and use that value to check for permission to edit meta.
4. Now it's capabilities fairy: current_user_can() calls WP_User::has_cap() which calls map_meta_cap() which checks for the term existence with a simple boolean test on the result of get_term().
5. But, because of $object_id value being 0, get_term() returns a WP_Error instance, the check mentionned in 4. fails, and we get the "Undefined property" notice that end up in the REST API request response.

TL;DR: Two issues here:

1. WP_REST_Terms_Controller::create_item() passes an unexisting value to the meta_value upate methods, indirectly triggering a bug in map_meta_cap();
2. map_meta_cap() does not break as it should if get_term() returns an error.

[caercam]

Added patch and opened a new ticket for map_meta_cap() issue: #40891

[TimothyBlynJacobs]

I think a better way to patch this would be to pass the term_id to the meta controller as opposed to updating the request object. This would match both how the update_item callback is written and how the [Post Controller ​https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L598] works and would keep the request object unmodified which might lead to some issues otherwise.

For example, a simple approach for writing an additional field for the REST API would be to check against the presence of an id parameter to determine if it is an update or a create request ( matching WP internals ). Checking against the HTTP method would probably be better, but I can easily imagine someone writing code like this.

[caercam]

Good point. Using $term->term_id was my initial approach, but then I figured having an undefined $request['id'] might be mistakenly interpreted as a failure to create the term even though it was properly inserted.

You're right a missing $request['id'] value could be used to determine if the term was created, though I'd personally go with a HTTP method check; that being said, WP_REST_Server::EDITABLE also supports POST in addition to PUT and PATCH, which may be cause more confusions yet to distinguish updates from creations. I'll add a patch using​ $term->term_id instead.

[jnylen0]

It would be good to get a PHPUnit test case that fails before this behavior and passes after. This will both ensure that the bug doesn't return in the future and make the ticket much easier to review and commit.

[rmccue]

@caercam Just a heads up: uploading a patch doesn't send notifications, so you should be sure to comment when you upload a new patch as well. :)

Patch looks good, but the test should check get_post_meta and confirm the value is actually set, rather than just the response code.

[theorboman]

Patch with Unit tests för the Categories and Tags controllers

[theorboman]

@rmccue @caercam here's the patch I wrote at contributor's day for this with unit tests for the tags and categories controllers.

[danielbachhuber]

Thanks for your work on the patches, @theorboman @caercam

This particular issue ended up being fixed by #44834

• Theme Json: Don't output double selectors for elements inside blocks #40889
• Global Styles: Load block CSS conditionally #41160

Trac ticket:

While trying to fix the tests I found a bug that wasn't caught in GB because the test is only on core, not in the GB repo. I'm going to try and patch that so we can add it to this one or we could "hack" the snapshot and then patch afterwards. I'm more inclined to do the former.

​This is the fix for the issue that the tests uncovered.

this only needs a review and check what's breaking the tests (they are all passing locally)

The tests previously ​failed on PHP 5.6 with a fatal error:
Fatal error: Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in src/wp-includes/class-wp-theme-json.php on line 1526

0015d99 should resolve the issue. We already used array_key_exists( $element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) in one place, and that resolves the error in other instances in my testing.

Thanks for the PR! Merged in r54118.

Thanks a lot, folks!

I was alerted that unfortunately, r54118 caused a fatal error on websites running Core trunk and a current version of the Gutenberg plugin 😕

PHP Fatal error:  Cannot redeclare wp_add_global_styles_for_blocks() (previously declared in wp-includes/global-styles-and-settings.php:202) in wp-content/plugins/gutenberg/lib/compat/wordpress-6.1/get-global-styles-and-settings.php on line 12

We'll need to add a ! function_exists() wrapper around the ​function definition in Gutenberg to avoid this.

Unfortunately, this won't fix the problem immediately: WP sites running Core trunk with the GB plugin active will continue to be affected until a new version of the plugin is released (as planned for next Thursday); or until the change is merged into GB trunk (if the site is running GB trunk). And finally, sites running Core trunk and an older version of GB (that has the unguarded wp_add_global_styles_for_blocks function definition) will continue to be affected 😕

I'm not sure how to best cover the latter case. For mitigation, we might want to consider reverting r54118 (as much as it pains me to suggest that) -- at least until a GB version with the fix is released.

---

Going forward, we should probably put some strategies in place to avoid collisions like this:

• Require wp_ function definitions in GB to be guarded with a ! function_exists() check (and ideally enforce with a WPCS lint).
• Add a check in wordpress-develop's CI to install and enable GB alongside Core (and run some basic tests to make sure it doesn’t fatal).

Anyway, this is something we'll discuss separately. I'll file one or two issues tomorrow.

Update: Unbeknownst to me, the wp_ prefix has been changed to gutenberg_ in the GB plugin: ​https://github.com/WordPress/gutenberg/pull/44052

Update #2: Let's hold off on the revert. We might release a Gutenberg point release with ​this fix as a stop-gap.