Skip to content

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.

}

/**
* 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

Development

Successfully merging this pull request may close these issues.

4 participants