google / google/site-kit-wp

Update all REST routes to make use of WordPress' REST schema capabilities

Open
#6,760 2 comments 0 reactions 0 assignees View on GitHub
P2 PHP Type: Enhancement
Dominant language
JavaScript
Stars
1.4k
Forks
383
Avg merge
4d 14h
Merged PRs (30d)
77

Description

## Feature Description

At present, we don't consistently make use of WordPress' REST schema capabilities to describe/validate REST route arguments and responses. These capabilities are useful and also time-saving compared to hand writing validation code.

We should update all of the routes in the plugin to make use of these capabilities, and ensure we consistently apply them to new routes going forward.

---------------

_Do not alter or remove anything below. The following sections will be managed by moderators only._

## Acceptance criteria

* Every REST route the plugin registers declares an argument schema for the parameters it accepts and a resource schema for the data it returns.
* Routes that return the same data structure share a single resource schema.
* A request with a missing required parameter, a parameter of the wrong type, or a value outside the set the parameter allows is rejected with a 400 response before the route's handler runs.
* Each route's argument and resource schemas appear in the site's REST API index.
* Every dashboard, setup, settings, sharing and email reporting flow continues to work unchanged.
* This issue should be treated as a time box which sets a hard limit on the time to spend.
* This will take multiple PRs, so follow-up issues should be opened as necessary until every route is covered.

## Implementation Brief

* [ ] In `includes/Core/REST_API/Schema/` (new directory):
* Add one class per resource shape that more than one route returns, each exposing a static `get()` that returns the schema array. Start with the shapes shared across dismissals, expirable items, surveys and module settings.
* [ ] Apply the same treatment in each controller below:
* Declare every accepted parameter in the endpoint `args` with `type`, `description`, `required`, and `enum` where the value set is fixed, nesting the properties of the `data` object for `POST` routes as `REST_Key_Metrics_Controller` already does.
* Pass a `schema` callback on the route describing the response, reusing a class from `Schema/` where two routes return the same shape.
* Remove the hand-written parameter checks the `args` schema now covers, and leave the checks it does not: permissions, unknown module slugs, inactive modules, and values that can only be validated against stored state.
* Where a hand-written check is removed, the response code for that failure changes from `missing_required_param` to WordPress' `rest_missing_callback_param`, or from a custom code to `rest_invalid_param`. Update the assertions in the affected tests.
* [ ] Dismissals, prompts, expirables and feature tours:
* `includes/Core/Dismissals/REST_Dismissals_Controller.php`
* `includes/Core/Prompts/REST_Prompts_Controller.php`
* `includes/Core/Expirables/REST_Expirable_Items_Controller.php`
* `includes/Core/Feature_Tours/REST_Feature_Tours_Controller.php`
* [ ] Per-user settings:
* `includes/Core/Key_Metrics/REST_Key_Metrics_Controller.php`
* `includes/Core/User_Input/REST_User_Input_Controller.php`
* `includes/Core/User/REST_Audience_Settings_Controller.php`
* `includes/Core/User/REST_Conversion_Reporting_Controller.php`
* `includes/Core/User/REST_Email_Reporting_Controller.php`
* `includes/Core/User/REST_Initial_Setup_Controller.php`
* `includes/Core/Tracking/REST_Tracking_Consent_Controller.php`
* [ ] Surveys:
* `includes/Core/User_Surveys/REST_User_Surveys_Controller.php`
* [ ] Site settings and status:
* `includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php`
* `includes/Core/Conversion_Tracking/REST_Conversion_Tracking_Controller.php`
* `includes/Core/Tags/Google_Tag_Gateway/REST_Google_Tag_Gateway_Controller.php`
* `includes/Core/Admin_Bar/Admin_Bar.php`
* `includes/Core/Site_Health/REST_Site_Health_Controller.php`
* `includes/Core/Util/Health_Checks.php`
* `includes/Core/Util/Developer_Plugin_Installer.php`
* `includes/Core/Util/REST_Entity_Search_Controller.php`
* [ ] Email reporting:
* `includes/Core/Email_Reporting/REST_Email_Reporting_Controller.php`
* [ ] Authentication, permissions and notifications:
* `includes/Core/Authentication/REST_Authentication_Controller.php`
* `includes/Core/Permissions/Permissions.php`
* `includes/Core/Nonces/Nonces.php`
* `includes/Core/Notifications/Notifications.php`
* `includes/Core/Util/Reset.php`
* [ ] Modules:
* `includes/Core/Modules/REST_Modules_Controller.php`, extending the existing module resource schema to the routes that do not yet pass one.
* `includes/Core/Modules/REST_Dashboard_Sharing_Controller.php`
* [ ] Module data endpoints:
* Implement `Schema_Aware_Datapoint` on each datapoint class under `includes/Modules/*/Datapoints/`, so it is registered as its own route rather than through the catch-all.
* Convert the datapoint definitions still declared as plain arrays in `includes/Modules/Search_Console.php`, `includes/Modules/Tag_Manager.php`, `includes/Modules/Site_Verification.php`, `includes/Modules/PageSpeed_Insights.php` and `includes/Modules/Sign_In_With_Google.php` into datapoint classes first.
* Keep the `Missing_Required_Param_Exception` guards inside the datapoint classes. `Module::get_data()` is also called directly from PHP, for example from `Analytics_4::get_google_tag_settings()`, where REST validation does not run.
* [ ] Take one group per PR, in the order above, and open a follow-up issue for each group not covered within the time box.

### Test Coverage

* [ ] For each controller converted, extend its existing test under `tests/phpunit/integration/` with:
* a request missing a required parameter returns a 400 and the handler is never reached.
* a parameter of the wrong type, or a value outside its `enum`, returns a 400.
* a valid request returns the same response as before the conversion.
* [ ] For each datapoint converted, extend the module's existing datapoint test with the same three cases, plus a check that the datapoint is served by its own route rather than the catch-all.
* [ ] In `tests/phpunit/integration/Core/REST_API/REST_RoutesTest.php`:
* every route registered under `google-site-kit/v1` exposes a resource schema.
* every registered route's argument schema is valid, using `rest_get_endpoint_args_for_schema()` or an equivalent structural check.
* [ ] Update the PHPUnit assertions that expect `missing_required_param` for checks moved into the argument schema.

## QA Brief

*

## Changelog entry

*

Contributor guide

Open the contributing guide

Research direction

Start with tests/phpunit/integration/Core/REST_API/REST_RoutesTest.php and one controller group, such as the dismissal and prompt controllers listed in the implementation brief. Read the existing endpoint args, schemas, and integration tests before converting that group. Done means its routes expose valid argument and resource schemas, invalid requests are rejected before handlers run, and valid responses remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
php, wordpress
Domain
api, backend-api-design, testing
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.