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

fix AMP compatibility for Standard and Transitional mode (#181) #182

Merged
merged 3 commits into from
Oct 17, 2020

Conversation

stklcode
Copy link
Contributor

Standard and Transitional mode use the amp_analytics_entries hook, Reader mode uses amp_post_template_analytics.
We only added a filter for the latter, so AMP compatibility was only available in Reader mode.

Standard and Transitional mode use the "amp_analytics_entries" hook,
Reader mode uses "amp_post_template_analytics". We only added a filter
for the latter, so AMP compatibility was only available in Reader mode.
@stklcode stklcode linked an issue Aug 28, 2020 that may be closed by this pull request
@stklcode stklcode added the bug label Aug 28, 2020
@stklcode stklcode added this to the 1.8.1 milestone Aug 28, 2020
@MatzeKitt
Copy link
Member

This works for me. 👍🏻

@stklcode
Copy link
Contributor Author

Great. Thanks for the quick feedback.

I‘ll add a condition to remove the unnecessary scripts tomorrow (if nobody is faster), then we can rollout the fix.

Scripts and inline JS get removed on pages served via AMP. While that's
fine for Statify, because we do use a separate analytics hook, it raises
warnings for invalid markup on every AMP page. We now detect AMP usage
and skip embedding the script.

Also the checks are updated to use the new amp_is_request() function
introduced in AMP plugin 2.0 for future compatibility.
@stklcode stklcode force-pushed the fix/181-amp-compatibility branch from 23fc7c2 to a19a01a Compare August 29, 2020 09:17
@stklcode
Copy link
Contributor Author

I've added the necessary check within the Statify_Frontend::wp_footer() method. Along with this change we now check for amp_is_request() with fallback to 'is_amp_endpoint()` (deprecated in AMP plugin 2.0).

An earlier check, e.g. not register wp_footer hook at all in the Statify::init() phase is not possible this way, because the AMP check is not initialized that early, so we jump out in the very first check if the actual hook.

@stklcode stklcode marked this pull request as ready for review August 29, 2020 09:22
@MatzeKitt
Copy link
Member

Also the check in wp_footer() is working for me. Thank you. 👍🏻

Commonly used methods "beacon" and "xhrpost" are enabled by default,
but as Statify doesn't work with "image", i.e. GET requests, we now
specify the supported methods explicitly again.
Copy link
Member

@florianbrinkmann florianbrinkmann left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Zodiac1978 Zodiac1978 merged commit 929231f into develop Oct 17, 2020
@stklcode stklcode deleted the fix/181-amp-compatibility branch October 17, 2020 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid markup by AMP plugin with enabled JavaScript tracking
4 participants