Directory

Support checking for a REST API request before the query is parsed by TimothyBJacobs · Pull Request #916 · WordPress/wordpress-develop · GitHub
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support checking for a REST API request before the query is parsed #916

Closed
wants to merge 1 commit into from

Conversation

TimothyBJacobs
Copy link
Member

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


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.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

I've added a couple of comments to this, having some trouble working out how to test locally so I be sure about the parsing. As it comes from the main WP class I assume it's pretty well battle tested though.

@@ -1490,6 +1490,96 @@ function wp_doing_cron() {
return apply_filters( 'wp_doing_cron', defined( 'DOING_CRON' ) && DOING_CRON );
}

/**
* Determines whether the current request is a REST API request.
Copy link
Contributor

Choose a reason for hiding this comment

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

CS Nit

Suggested change
* Determines whether the current request is a REST API request.
* Determine whether the current request is a REST API request.

Comment on lines +1511 to +1512
// This intentionally doesn't use wp_using_themes(). We are trying to check if wp() would be called for this request
// in a front-end context which is typically indicated by the WP_USE_THEMES constant.
Copy link
Contributor

Choose a reason for hiding this comment

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

/*
 * CS: multiline
 * comment format
 */

🔢

$index = 'index.php';
}

$pathinfo = isset( $_SERVER['PATH_INFO'] ) ? $_SERVER['PATH_INFO'] : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a mostly a copy-paste from WP but a couple of tiny things could be tidied up along the way.

Suggested change
$pathinfo = isset( $_SERVER['PATH_INFO'] ) ? $_SERVER['PATH_INFO'] : '';
$path_info = isset( $_SERVER['PATH_INFO'] ) ? $_SERVER['PATH_INFO'] : '';

🔢

$pathinfo = str_replace( '%', '%25', $pathinfo );

list( $req_uri ) = explode( '?', $_SERVER['REQUEST_URI'] );
$home_path = trim( parse_url( home_url(), PHP_URL_PATH ), '/' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Core helper if it's defined when this runs.

Suggested change
$home_path = trim( parse_url( home_url(), PHP_URL_PATH ), '/' );
$home_path = trim( wp_parse_url( home_url(), PHP_URL_PATH ), '/' );

Copy link

Choose a reason for hiding this comment

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

If the return value of home_url() does not contain a path (e.g. "https://example.com") then in PHP 8.1+ this results in a "trim(): Passing null to parameter #1 ($string) of type string is deprecated" warning.

Thus, this should probably be replaced with something like:

$home_url_path   = wp_parse_url( home_url(), PHP_URL_PATH );
$home_path       = $home_url_path ? trim( $home_url_path, '/' ) : '';

@felixarntz
Copy link
Member

While the purpose of this PR is different, in terms of https://core.trac.wordpress.org/ticket/42061 it is now superseded by #5658.

We may want to open another ticket for the specific point of checking for a REST API request prior to parsing the query, as that's a more specific use-case than what was mostly discussed on the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants