Repository navigation
Abilities API: Encode empty object results as JSON objects - #14547
jorgefilipecosta wants to merge 1 commit into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
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. |
core/settings-get when no setting matches
8e657ba to
29c2ab8
Compare
|
I noted in #14139 that we should follow-up and figure out how to map |
29c2ab8 to
0437a0b
Compare
core/settings-get when no setting matches0437a0b to
e68fd10
Compare
core/settings-get when no setting matches
e68fd10 to
d05b08b
Compare
core/settings-get when no setting matchesd05b08b to
9d63d51
Compare
Hi @gziolo this PR was updated and addressed these issues. |
|
Could we let controllers explicitly opt out of The problem extends beyond objects to scalar values, where these transformations have no meaningful application. Currently, The ability execution controller could disable all three explicitly, preserving the ability’s output schema and making behavior predictable regardless of whether the result is an object, array, or scalar. |
9d63d51 to
374b904
Compare
gziolo
left a comment
There was a problem hiding this comment.
A few suggestions to make the test cases easier to follow and strengthen coverage of the response-preservation behavior.
|
|
||
| $data = array(); | ||
| foreach ( $results as $label => $result ) { | ||
| foreach ( array( 'none', '_fields', '_embed', 'link', 'embedded link' ) as $global_param ) { |
There was a problem hiding this comment.
$global_param contains both query parameter names and instructions for setting up the test, such as link and embedded link. This makes the cases harder to follow.
Could we consider passing query parameters directly from the provider, for example array( '_fields' => 'other', '_embed' => '1' ), and applying them with $request->set_query_params()? Link setup could be a separate argument. This would also make it easy to test both parameters together.
| add_filter( | ||
| 'rest_post_dispatch', | ||
| static function ( $response ) { | ||
| $response->add_link( 'alternate', rest_url( '/wp-abilities/v1/abilities' ), array( 'embeddable' => true ) ); |
There was a problem hiding this comment.
This test adds a link to the response metadata. Since link injection is disabled, that link never reaches the response body. The test therefore does not check what happens when the ability’s output already contains _links.
Could we consider adding a case where the ability returns its own _links property with an embeddable link? With _embed=1, we could check that the output stays unchanged and no request is made to fetch the linked resource.
|
|
||
| add_filter( 'rest_post_dispatch', 'rest_filter_response_fields', 10, 3 ); | ||
| $response = apply_filters( 'rest_post_dispatch', $response, $this->server, $request ); | ||
| $data = $this->server->response_to_data( $response, isset( $global_params['_embed'] ) ); |
There was a problem hiding this comment.
This test manually calls the steps that process and encode the response. It checks those steps, but it does not check the body sent by serve_request().
Could we consider using serve_request() for this regression test, passing the input and query parameters through the normal request path and checking the emitted JSON? We could verify that the body is {} when _fields and _embed are supplied together. The broader output-shape tests could keep their current approach.
| 'numeric collection stays unchanged' => array( 'test/empty-object-list-output', '[[]]' ), | ||
| ); | ||
| $results = array( | ||
| 'root empty object' => array( 'test/empty-object-output', '{}' ), |
There was a problem hiding this comment.
This case checks that a root empty object is encoded as {} when the response is served. Preloaded requests take a different path. rest_preload_api_request() casts the data with (array), and that turns the stdClass back into an empty array. So the preloaded body becomes [], and a scalar like "value" becomes ["value"].
Could we consider adding a preload test for a read-only ability (preloading only allows GET) that returns an empty object and a scalar? Today it would fail for both. The fix could be to skip the (array) cast in rest_preload_api_request(), either here or in #14671 since that PR covers preloaded requests.
374b904 to
cd02d31
Compare
|
I approved #14671, so let's commit it, and I will finish reviewing this one. Currently, it contains changes from both. |
|
@jorgefilipecosta, do we still want to include this in WordPress 7.2? |
The run endpoint prepared only associative array results for JSON
clients, so an empty array returned for an object output schema, such as
the `core/settings-get` result when no setting matches, was encoded as []
and failed output validation in JSON clients. Prepare every result,
including root results, lists and object results, so empty objects are
encoded as {} while arrays stay arrays.
Register the run endpoint with the `data_processing` route option set to
`false`, so the `_fields` and `_embed` parameters and response links leave
the result unchanged and it follows the ability's output schema whether it
is an object, an array, or a scalar value. The REST API client fixture
lists the disabled processing for the run endpoint.
Follow-up to [64311], [64592], [64689].
See #66267.
cd02d31 to
0ac67da
Compare
Follow up to https://core.trac.wordpress.org/changeset/64592 (#14139) and https://core.trac.wordpress.org/changeset/64689 (#14671).
What
wp_prepare_json_value_for_client(), not only associative arrays. An empty array returned for an object output schema is now encoded as{}at the root, in lists, and inside object orJsonSerializableresults, instead of[], which the JS client (executeAbility()) rejected withability_invalid_output. This fixes it for all abilities, e.g.core/settings-getwhen no setting matches.[].data_processingroute option (from https://core.trac.wordpress.org/changeset/64689) tofalse, so_fields,_embedand response links leave the result unchanged, and it follows the ability's output schema whether it is an object, an array, or a scalar value. The REST API client fixture lists the disabled processing for the run endpoint.Issue found while testing the query abilities through the JS client; the fix was drafted with AI assistance and reviewed by me.
Trac ticket: https://core.trac.wordpress.org/ticket/66267
Testing
Verify unit tests are passing: