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

Set AMP_QUERY_VAR fallback when AMP is inactive #986

Merged
merged 2 commits into from
Mar 12, 2018

Conversation

mjangda
Copy link
Contributor

@mjangda mjangda commented Mar 2, 2018

If AMP is deactivated via the amp_is_enabled filter, the AMP_QUERY_VAR ends up undefined. This causes warnings when a helper function like is_amp_endpoint() is called. Setting a fallback value prevents this.

This was happening with a site running both amp-wp (but deactivated via the filter) and lazy-load (which calls is_amp_endpoint()):

Warning: Use of undefined constant AMP_QUERY_VAR - assumed 'AMP_QUERY_VAR' (this will throw an Error in a future version of PHP) in wp-content/plugins/amp/includes/amp-helper-functions.php

To reproduce:

add_filter( 'amp_is_enabled', '__return_false' );

add_action( 'wp_head', function() {
    is_amp_endpoint();
} );

@mjangda
Copy link
Contributor Author

mjangda commented Mar 2, 2018

We may also want to consider hardening up the helper functions as well to return appropriate values when AMP is disabled via the filter.

@westonruter
Copy link
Member

See also #945. What have been the use cases for AMP_QUERY_VAR in the wild?

@mjangda
Copy link
Contributor Author

mjangda commented Mar 2, 2018

One example is http://indianexpress.com/article/india/breakthrough-journalist-gauri-lankesh-murder-first-arrest-5084078/lite/ (they use lite instead of amp).

Deprecation is fine too :)

@westonruter
Copy link
Member

westonruter commented Mar 2, 2018

We may also want to consider hardening up the helper functions as well to return appropriate values when AMP is disabled via the filter.

I think this might be the better way to go instead of defining AMP_QUERY_VAR as false, as if someone does later define AMP_QUERY_VAR then they'll get a notice about it already being defined. Or rather, if changing the slug is to be deprecated, then maybe we should just stop using AMP_QUERY_VAR altogether and instead introduce an amp_get_slug() function instead.

mjangda and others added 2 commits March 9, 2018 21:10
If AMP is deactivated via the `amp_is_enabled` filter, the
`AMP_QUERY_VAR` ends up undefined. This causes warnings when a helper
function like `is_amp_endpoint()` is called.

Setting a fallback value prevents this.
@westonruter
Copy link
Member

Rebased to fix merge conflict. Former head was 82dced4.

@mjangda
Copy link
Contributor Author

mjangda commented Mar 10, 2018

we should just stop using AMP_QUERY_VAR altogether and instead introduce an amp_get_slug() function instead.

Sounds like a good idea.

@westonruter westonruter added this to the v0.7 milestone Mar 10, 2018
@ThierryA
Copy link
Collaborator

@westonruter I see this PR is now approved, does it still need my review?

@westonruter
Copy link
Member

@ThierryA I just wanted you to be aware of it.

@westonruter westonruter merged commit c402da5 into develop Mar 12, 2018
@westonruter westonruter deleted the add/QUERY_VAR-fallback branch March 12, 2018 16:48
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.

3 participants