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 checks for serving AMP response and improve AMP compatibility #10918

Closed

Conversation

westonruter
Copy link
Contributor

Jetpack stats are currently broken for sites running 1.0 of the AMP plugin. See ampproject/amp-wp#1722 and https://wordpress.org/support/topic/v1-0-has-broken-jetpack-stats-tracking/

The problem is that the plugin was not updated after ampproject/amp-wp#1148 (comment) was done during the AMP plugin's beta releases.

To fix, Jetpack needs to wait until after parse_query action has happened in order to determine whether an AMP response is being served.

This PR also:

  • Eliminates the omission of the Milestone widget in AMP; instead the widget's output is modified to make it AMP compatible.
  • Jetpack stats in the admin bar are made AMP compatible.

Closes #9588.
See also #9730.

/cc @gravityrail

@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against c83dd8c

@jeherve jeherve added [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. AMP labels Dec 10, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 10, 2018
Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

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

I get a lot of errors like is_amp_endpoint was called incorrectly, caused by some of these hooked functions which are run during init, before parse_query.

Culprits:

invoked directly in likes.php line 272
filter jp_carousel_maybe_disable invoked in jetpack-carousel.php line 102
probably more...

@gravityrail
Copy link
Contributor

I made a modified version of this PR in #10945

@kraftbj kraftbj removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Feature] Stats Data Feature that enables users to track their site's traffic and gain insights on popular content. Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for /amp/ suffix when determining is_amp_request
6 participants