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

Projects
None yet
3 participants
@mjangda
Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Collaborator

westonruter commented Mar 2, 2018

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

@mjangda

This comment has been minimized.

Member

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

This comment has been minimized.

Collaborator

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 some commits Mar 2, 2018

Set AMP_QUERY_VAR fallback when AMP is inactive
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

This comment has been minimized.

Collaborator

westonruter commented Mar 10, 2018

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

@mjangda

This comment has been minimized.

Member

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 requested a review from ThierryA Mar 10, 2018

@westonruter westonruter added this to the v0.7 milestone Mar 10, 2018

@ThierryA

This comment has been minimized.

Collaborator

ThierryA commented Mar 12, 2018

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

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Mar 12, 2018

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

@westonruter westonruter merged commit c402da5 into develop Mar 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/QUERY_VAR-fallback branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment