Skip to content

Fix checks for serving AMP response and improve AMP compatibility#10918

Closed
westonruter wants to merge 3 commits intoAutomattic:masterfrom
westonruter:fix/amp-endpoint-check
Closed

Fix checks for serving AMP response and improve AMP compatibility#10918
westonruter wants to merge 3 commits intoAutomattic:masterfrom
westonruter:fix/amp-endpoint-check

Conversation

@westonruter
Copy link
Copy Markdown
Contributor

@westonruter westonruter commented Dec 10, 2018

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

@westonruter westonruter requested a review from a team December 10, 2018 18:35
@westonruter westonruter mentioned this pull request Dec 10, 2018
Closed
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Dec 10, 2018

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 This PR is ready for review. AMP labels Dec 10, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 10, 2018
gravityrail
gravityrail suggested changes Dec 11, 2018
View reviewed changes
Copy link
Copy Markdown
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 gravityrail mentioned this pull request Dec 11, 2018
Merged
@gravityrail
Copy link
Copy Markdown
Contributor

gravityrail commented Dec 11, 2018

I made a modified version of this PR in #10945

@kraftbj kraftbj removed the [Status] Needs Review This PR is ready for review. label May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gravityrail gravityrail gravityrail requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

6.9

Development

Successfully merging this pull request may close these issues.

Check for /amp/ suffix when determining is_amp_request

6 participants

@westonruter @jetpackbot @gravityrail @kraftbj @jeherve @matticbot