Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Use core WP's query var allow-list #131

Merged
merged 2 commits into from
Apr 10, 2020
Merged

Use core WP's query var allow-list #131

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45c0418
Use core WP's query var allow-list
jameswburke Apr 9, 2020
ea9daa8
Address phpcs errors
jameswburke Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions inc/endpoints/class-components-endpoint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,23 @@ public function register_rest_routes() {
* @return array
*/
public function get_route_response( $request ) {

// phpcs:ignore WordPress.WP.GlobalVariablesOverride.OverrideProhibited
global $wp;

/**
* Action fired on the request.
*
* @param \WP_REST_Request $request WP_REST_Request object.
*/
do_action( 'wp_irving_components_request', $request );

$this->params = $request->get_params();
// Remove any request parameters that haven't been allow-listed by the
// global $wp object's $public_query_vars array.
$this->params = array_intersect_key(
$request->get_params(),
array_flip( $wp->public_query_vars )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that is clever

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the full list of where WP adds public query vars:
https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp.php#L9-L17

Are we sure we want/need to support all of these explicitly? Many of them should be handled by the route parsing on the WP side, I would think.

);

// Parse path and context.
$this->parse_path( $this->params['path'] ?? '' );
Expand Down Expand Up @@ -366,7 +375,7 @@ public function build_query() {
$wp_query = apply_filters( 'wp_irving_components_wp_query', $wp_query, $this->path, $this->custom_params, $this->params );

// Map to main query and set up globals.
// phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
// phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited, WordPress.WP.GlobalVariablesOverride.OverrideProhibited
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the sniff rule change, or do we really need to be ignoring both here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I know enough about each of those vars to make an educated decision on whether or not they should be present. I do believe we should be mimicking WordPress core as much as possible though.

One of those phpcs rules hits me locally, one hits me on Travis, so I included both for the sake of it right now.

$wp_the_query = $wp_query;
$this->register_globals();

Expand Down Expand Up @@ -396,7 +405,7 @@ public function permissions_check( $request ) {
* @see https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp.php#L580
*/
public function register_globals() {
// phpcs:disable WordPress.WP.GlobalVariablesOverride.Prohibited
// phpcs:disable WordPress.WP.GlobalVariablesOverride.Prohibited, WordPress.WP.GlobalVariablesOverride.OverrideProhibited
global $wp_the_query, $wp_query;
$wp_query = $wp_the_query;

Expand Down Expand Up @@ -442,6 +451,8 @@ public function fix_rest_url( $url ) : string {
* @return array $vars Array of query vars.
*/
public function modify_query_vars( $vars ) {
$vars[] = 'context';
$vars[] = 'path';
$vars[] = 'irving-path';
$vars[] = 'irving-path-params';
return $vars;
Expand Down