First pass at batch controller

[TimothyBJacobs]

Trac ticket: https://core.trac.wordpress.org/ticket/50244

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[adamziel]

Adding a support for sanitize_callback would make things even easier on the controller side. It could be done by adding the following code to sanitize_params method in class WP_REST_Request:

                if ( isset( $attributes['sanitize_callback'] ) ) {
                        $sanitized = call_user_func( $attributes['sanitize_callback'], $this );

                        if ( is_wp_error( $sanitized ) ) {
                                return $sanitized;
                        }
                }

[TimothyBJacobs]

Why sanitize? The sanitize callbacks aren’t supposed to return an error instance.

[adamziel]

Why sanitize?

@TimothyBJacobs How else would you approach classifying e.g. this part of the class-wp-rest-menu-items-controller.php?

https://github.com/WordPress/gutenberg/blob/b1ffaf373e6b2604fcaded300e8f8b445d4237df/lib/class-wp-rest-menu-items-controller.php#L516-L527

It performs validation based on:

• Menu item stored in the database
• Results of sanitization of menu-item-type earlier in the same function

It doesn't neatly fit into the validation callback, but it fits the sanitize callback, with the only caveat being returning an error.

The sanitize callbacks aren’t supposed to return an error instance.

Returning an error instance is supported for per-field validated callback so my thinking was that a per-request callback should also support it.

My bigger problem here is that the notion of validation and sanitization in the API relates to request data, whereas with endpoints like class-wp-rest-menu-items-controller.php what we really want to validate/sanitize is a hydrated object which is a mixture of request data and database data.

[TimothyBJacobs]

How else would you approach classifying e.g. this part of the class-wp-rest-menu-items-controller.php?

It doesn't neatly fit into the validation callback, but it fits the sanitize callback, with the only caveat being returning an error.

Why doesn't it fit in validate? The purpose of sanitize is to transform a value, AFAICT it isn't transforming a value, but validating that it is valid when combined with other request data.

Returning an error instance is supported for per-field validated callback so my thinking was that a per-request callback should also support it.

Right, but I think that would be in the per-request validate_callback no?

[adamziel]

@TimothyBJacobs here's my reasoning:

Consider the following block of code:

https://github.com/WordPress/gutenberg/blob/2e80a87f2bedd22a4b30f1aa10421d7230ff1e82/lib/class-wp-rest-menu-items-controller.php#L393-L411

What it does is this: If menu-item-object is missing from $prepared_nav_item, then fill it based on menu-item-type and menu-item-object-id.

That code depends on $prepared_nav_item which is based on both request and database data, let's call it a hydrated $prepared_nav_item.

Let's try to split that block of code assuming we only have the following buckets to choose from:

1. per-field validate_callback
2. per-field sanitize_callback
3. per-request validate_callback
4. Route callback

Let's see how it fits each of these buckets:

• per-field validate_callback: menu-item-object is not a request field so there is no related validate_callback. Let's consider reusing menu-item-type validate_callback. It would need to hydrate $prepared_nav_item first, and then validate conditionally on menu-item-object being falsy. That doesn't seem right.
• per-field sanitize_callback: Same as above, plus there isn't a good way to account for this line: $prepared_nav_item['menu-item-object'] = get_term_field( 'taxonomy', $original );.
• per-request validate_callback: Hydrating $prepared_nav_item and returning new WP_Error in the error case would work just fine. There is no good way to account for this line though: $prepared_nav_item['menu-item-object'] = get_term_field( 'taxonomy', $original );
• Route callback: Provided returning an error was handled by the validate_callback, we could hydrate $prepared_nav_item again and duplicate most of the logic/ifs to conditionally assign $prepared_nav_item['menu-item-object'] = get_term_field( 'taxonomy', $original );. This means we need to hydrate twice and duplicate some of the validation logic in the controller to only set menu-item-object under specific conditions. It's okay-ish but not ideal.

There is also one additional problem. See the following code:

https://github.com/WordPress/gutenberg/blob/2e80a87f2bedd22a4b30f1aa10421d7230ff1e82/lib/class-wp-rest-menu-items-controller.php#L413-L420

It performs validation based on the value of resolved $prepared_nav_item['menu-item-object'] from the snippet above. That logic does not nicely follow a linear pattern like 1. Throw errors for invalid inputs 2. Transform the validated data to a save-able form. It's more like an interdependent mixture of both. There are other parts (like menu position logic) that fit the above reasoning too.

Now let's see what would happen with per-request sanitize_callback (or hydrate_callback, transform_callback, prepare_item_for_database, or any other name really):

sanitize_callback hydrates $prepared_nav_item, performs a check and return an error if needed, assigns $prepared_nav_item[ "menu-item-object" ], and then makes the $prepared_nav_item available for the action handler to process without any further transformations (at least ones that have request-dependent error conditions). There is no need to hydrate twice, there is no duplicate code, and it may be performed in bulk before any request callback kicks in.

It seems cleaner overall, let me know what you think or if you have any alternative ideas - I don't insist on this specific approach, just trying to figure out what would work best here :)

[TimothyBJacobs]

I royally screwed up this rebase. Closing this out.