-
Notifications
You must be signed in to change notification settings - Fork 384
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
Refactor conditional checks in is_amp_endpoint() and improve guidance when _doing_it_wrong() #4574
Conversation
Moved checking against is_embed() and is_feed() after checking if parse_query has run and $wp_query has been filled, since both these functions will fire _doing_it_wrong when $wp_query is not yet set.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the pull request @hansschuijff !
There's a code style issue right now that prevents the actual tests from running. See my review comment on how to fix it.
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@hansschuijff It looks like there's no obvious way right now to add a regression test for the above fix. I've opened an issue to that regard: #4576 |
includes/amp-helper-functions.php
Outdated
@@ -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 ) ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
.
…g_it_wrong warnings
One more thing that comes to mind to improve this is to look at the call stack to find out which theme/plugin is actually responsible for calling |
…ition in redirects
includes/amp-helper-functions.php
Outdated
if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) { | ||
$availability = AMP_Theme_Support::get_template_availability( $wp_query ); | ||
$supported = $availability['supported']; | ||
} else { | ||
$queried_object = get_queried_object(); | ||
$supported = ( $wp_query->is_singular() || $wp_query->is_posts_page ) && $queried_object instanceof WP_Post && post_supports_amp( $queried_object ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early bails should be preferred over if/else constructs. This is easier to reason about and keeps the level of indentation down.
if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) { | |
$availability = AMP_Theme_Support::get_template_availability( $wp_query ); | |
$supported = $availability['supported']; | |
} else { | |
$queried_object = get_queried_object(); | |
$supported = ( $wp_query->is_singular() || $wp_query->is_posts_page ) && $queried_object instanceof WP_Post && post_supports_amp( $queried_object ); | |
} | |
if ( current_theme_supports( AMP_Theme_Support::SLUG ) ) { | |
$availability = AMP_Theme_Support::get_template_availability( $wp_query ); | |
return $availability['supported']; | |
} | |
$queried_object = get_queried_object(); | |
return ( $wp_query->is_singular() || $wp_query->is_posts_page ) && $queried_object instanceof WP_Post && post_supports_amp( $queried_object ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't provide a suggestion for the rest of this piece of code, as suggestions don't seem to work across deleted lines.
But the change should basically be from this:
if () {
if () {
$supported = ...
else {
$supported = ...
}
} else {
$supported = ...
}
return $supported;
...to this:
if () {
if () {
return ...;
}
return ...;
}
return ...;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 751c645
// @codeCoverageIgnoreStart | ||
if ( $exit ) { | ||
if ( wp_safe_redirect( $non_amp_url, $status ) ) { | ||
// @codeCoverageIgnoreStart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is code coverage being ignored here? This would warrant an explanatory comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because exit
cannot be tested.
Co-Authored-By: Alain Schlesser <[email protected]>
* 'develop' of github.com:ampproject/amp-wp: Update tests after block-library/style.css changes in Gutenberg 7.9 (ampproject#4579) Restrict doing plugin upgrade routine when not in admin (ampproject#4538)
@hansschuijff Sorry for increasing the scope of the PR so greatly. Your original change was very needed, and then it identified a whole bunch of other improvements that also needed to be made. |
@westonruter No worries. I'm glad I could help, even if it was just in a small way. Seeing you guys work is educational too and having done my first pr. Sometimes just initiating something is enough and glad it inspired to enhance amp even more. |
Moved checking against is_embed() and is_feed() after
checking if parse_query has run and $wp_query has been filled, since
both these functions will fire _doing_it_wrong when $wp_query
is not yet set.
Summary
Fixes #4525
Checklist