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.

Sign up for GitHub

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

Jump to bottom

Refactor conditional checks in is_amp_endpoint() and improve guidance when _doing_it_wrong() #4574

Merged
merged 13 commits into from
Apr 16, 2020
Merged

Refactor conditional checks in is_amp_endpoint() and improve guidance when _doing_it_wrong() #4574

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f346c04
fixed issue #4525
hansschuijff Apr 12, 2020
25366b7
Commit suggestion: end inline comment with .
hansschuijff Apr 12, 2020
fa4504f
Start comment with upper-case letter
westonruter Apr 13, 2020
0bdf639
Only check REST_REQUEST after parse_request; prevent duplicated _doin…
westonruter Apr 13, 2020
f22c4c7
Update version for _doing_it_wrong() from 1.5.3 to 1.6.0
westonruter Apr 13, 2020
27a5192
Add comment explaining short-circuit behavior when query var is present
westonruter Apr 13, 2020
b498d91
Fix translators comment
westonruter Apr 13, 2020
47926ce
Remove special conditions for Reader mode; remove need for $exit cond…
westonruter Apr 13, 2020
5995ee0
Fix typo in global phpdoc
westonruter Apr 13, 2020
d79097c
Add conditions for comment feed, trackback, robots, and favicon
westonruter Apr 14, 2020
d56b990
Improve phpdoc and logic conditions
westonruter Apr 14, 2020
751c645
Return early instead of storing eventual return value in variable
westonruter Apr 14, 2020
88e9403
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/4525
westonruter Apr 14, 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
7 changes: 6 additions & 1 deletion includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ function post_supports_amp( $post ) {
function is_amp_endpoint() {
global $pagenow, $wp_query;

if ( is_admin() || is_embed() || is_feed() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php' ], true ) ) {
if ( is_admin() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || in_array( $pagenow, [ 'wp-login.php', 'wp-signup.php', 'wp-activate.php' ], true ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that REST_REQUEST maybe being checked too early here. It is defined in the rest_api_loaded() function which is called at the parse_request action. So perhaps we need to add another check for whether the parse_request action was done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand it correctly, that seems to not lead to problems, since than REST_REQUEST just won't be defined and it will pass through the conditional and just fire _doing_it_wrong until the parse_request action is performed and the $wp_query is filled. Is_embed and is_feed just needed to move since otherwise the error reported had no obvious relation with is_amp_endpoint. It might be right to let the function report an error anyway if the function is called too early in the process, so that the plugin developers get to see where what they did wrong?

Copy link
Member

Choose a reason for hiding this comment

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

I think the issue is just that during REST API requests, the wp action is never triggered so it's somewhat misleading. In order to check the constant we need to check if the parse_request action was fired, or else it's not good to check this constant since it will never be defined. I'm going to push up a change to this to account for this and you can see what I mean.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I've rearranged the checks in 0bdf639 which I think will produce better results. This change will also ensure that a call to is_amp_endpoint() won't cause a warning in addition to warnings about calling is_embed() and is_feed().

return false;
}

Expand Down Expand Up @@ -603,6 +603,11 @@ function is_amp_endpoint() {
);
}

// note: is_embed() and is_feed() need $wp_query, so above checks must go first.
westonruter marked this conversation as resolved.
Show resolved Hide resolved
if ( is_embed() || is_feed() ) {
return false;
}

/*
* If this is a URL for validation, and validation is forced for all URLs, return true.
* Normally, this would be false if the user has deselected a template,
Expand Down