Fix checks for serving AMP response and improve AMP compatibility #10945
Conversation
|
D22154-code. (newly created revision) |
gravityrail
added
[Type] Bug
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: January 10, 2019. |
gravityrail
changed the title
jeherve
added
AMP
[Feature] Stats Data
There was a problem hiding this comment.
I deployed the changes to https://weston.ruter.net/ and I didn't notice any regressions or PHP log entries, though I don't have all modules active on my site.
jeherve
added
the
[Status] Needs Testing
|
Whoops! Thanks @westonruter |
|
Note that images on AMP pages are currently broken with the current version of Jetpack because lazy images aren't being disabled. This PR should be marked important, I believe. |
|
I have tested this one a bit, seems to be working fine, but I have a question. Can we just filter out modules instead of introducing many checks into their code? We can have something like this for a predefined list of modules that shouldn't even be loaded if it's an AMP render: We can have this as a static method in |
@zinigor I do not believe so because |
|
Oh, yes, you're right, it runs on |
|
I have found that with Masterbar enabled a logged in view seems to be way off, so we need to disable Masterbar as well. For that I had to change the way the module is initialized, @vindl can you please take a look and tell us if there are any caveats to doing this? The admin bar seems to work OK in non-AMP requests, but still, I'd love a second set of eyes. |
|
Meanwhile I'm going to work on the |
|
@zinigor Just to confirm, yes, the reason I filter functionality rather than modules is because when Jetpack is loading its modules, we don't know yet whether the page is going to be rendered as AMP. |
In order to synchronize code between Jetpack and WordPress.com better one of the little pieces we needed was the `is_module_active` method present in the Site Abstraction Layer. This change adds it as well as enforcing its existence in all extending classes. #### Changes proposed in this Pull Request: * Adds unit tests that will run on SAL classes. * Adds the `is_module_active` method for SAL Sites. #### Testing instructions: * Functionally there's nothing to change, just run the tests. * This should fail on wpcom check after being created, this is expected, because the method needs to be added on the other side as well. #### Proposed changelog entry for your changes: * Added base unit test suites for the SAL library. This blocks #10945
|
@dereksmart do you have stack traces for those? |
|
[03-Jan-2019 23:02:26 UTC] PHP Notice: Trying to get property 'post_type' of non-object in /home/dereksma/public_html/wp-content/plugins/amp/includes/class-amp-post-type-support.php on line 80 |
jeherve
added
[Status] Has Changelog
and removed
[Status] Needs Changelog
[Status] Needs Testing
|
@dereksmart Thanks. It seems Jetpack is doing the right thing: Lines 16 to 26 in c749591 Here's the relevant code in the AMP plugin: It seems that I'm confused why |
* Add first version of the Changelog and testing list for 6.9 * Changelog: add #10710 * changelog: add #10538 * changelog: add #10741 * changelog: add #10749 * changelog: add #10664 * changelog: add #10224 * changelog: add #10788 * Changelog: add #10560 * Chanegelog: add #10812 * changelog: add #10556 * Changelog: add #10668 * Changelog: add #10846 * Changelog: add #10947 * Changelog: add #10962 * Changelog: add #10956 * Changelog: add #10940 * Changelog: add #10934 * Changelog: add #10912 * changelog: add #10866 * changelog: add #10924 * Changelog: add #10936 * Changelog: add #10833 * changelog: add #10867 * Changelog: add #10960 * Changelog: add #10888 * changelog: add #10840 * changelog: add #10972 * Changelog: add #10979 * changelog: add #10909 * Changelog: add #10958 * Changelog: add #10981 * Changelog: add #10564 * Changelog: add #10809 * Changelog: add #10982 * Changelog: add #10706 * Changelog: add #10978 * Changelog: add #10132 * Changelog: add #11022 * Changelog: add #11024 * Changelog: add #10875 * Changelog: add #11030 * Changelog: add #11053 * Changelog: add #10880 * Changelog: add #9359 * Changelog: add #11037 * Update block list * Changelog: add #11060 * Changelog: add #10755 * changelog: add #11000 * Changelog: add #10786 * Changelog: add #10945 * Changelog: add #10597
|
Seems the problem is that @dereksmart If you do: - add_action( 'parse_query', 'jetpack_initialize_masterbar', 99 );
+ add_action( 'wp', 'jetpack_initialize_masterbar', 99 ); Does that have the intended result in paired mode, where the AMP version continues to hide the masterbar but in the non-AMP version, the masterbar is shown? |
|
It looks like we'll have to make these changes to the AMP plugin as well, to warn when calling before diff --git a/includes/amp-helper-functions.php b/wp-content/plugins/amp/includes/amp-helper-functions.php
index 938eb40b..4a29b021 100644
--- a/includes/amp-helper-functions.php
+++ b/includes/amp-helper-functions.php
@@ -252,10 +252,10 @@ function is_amp_endpoint() {
return false;
}
- $did_parse_query = did_action( 'parse_query' );
+ $did_parse_query = did_action( 'wp' );
if ( ! $did_parse_query ) {
- _doing_it_wrong( __FUNCTION__, sprintf( esc_html__( "is_amp_endpoint() was called before the 'parse_query' hook was called. This function will always return 'false' before the 'parse_query' hook is called.", 'amp' ) ), '0.4.2' );
+ _doing_it_wrong( __FUNCTION__, sprintf( esc_html__( "is_amp_endpoint() was called before the 'wp' hook was called. This function will always return 'false' before the 'wp' hook is called.", 'amp' ) ), '0.4.2' );
}
$has_amp_query_var = (
diff --git a/includes/class-amp-post-type-support.php b/wp-content/plugins/amp/includes/class-amp-post-type-support.php
index 3863b1f2..a1dad0c4 100644
--- a/includes/class-amp-post-type-support.php
+++ b/includes/class-amp-post-type-support.php
@@ -75,6 +75,9 @@ class AMP_Post_Type_Support {
if ( ! ( $post instanceof WP_Post ) ) {
$post = get_post( $post );
}
+ if ( ! $post ) {
+ return array();
+ }
$errors = array();
if ( ! post_type_supports( $post->post_type, self::SLUG ) ) {
diff --git a/includes/class-amp-theme-support.php b/wp-content/plugins/amp/includes/class-amp-theme-support.php
index 215ce42c..97357edb 100644
--- a/includes/class-amp-theme-support.php
+++ b/includes/class-amp-theme-support.php
@@ -586,10 +586,12 @@ class AMP_Theme_Support {
* @var WP_Post $queried_object
*/
$queried_object = $query->get_queried_object();
- $support_errors = AMP_Post_Type_Support::get_support_errors( $queried_object );
- if ( ! empty( $support_errors ) ) {
- $matching_template['errors'] = array_merge( $matching_template['errors'], $support_errors );
- $matching_template['supported'] = false;
+ if ( $queried_object ) {
+ $support_errors = AMP_Post_Type_Support::get_support_errors( $queried_object );
+ if ( ! empty( $support_errors ) ) {
+ $matching_template['errors'] = array_merge( $matching_template['errors'], $support_errors );
+ $matching_template['supported'] = false;
+ }
}
} |
|
I've opened an AMP plugin PR to warn when calling |
|
I moved this remaining problem to a new issue: #11082 |
|
Thanks for digging @westonruter!
seems to somewhat work. I still see the masterbar in AMP mode, but there are no notices. |
Summary: After adding the AMP compat file that ships with Jetpack (this was done in D22313, D26814, D26819, D26921, and D26928), let's start using this class. This replaces D22154 and D23501, and allows us to start syncing any future changes that would rely on the Jetpack_AMP_Support class. Related Jetpack PRs: - #10945 - #11195, which reverted some of the changes in the PR above. - #12054 - #12053 - #12026 Discussion: - https://[private link] We'll need to test for issues like the ones that popped up on Jetpack at the time: #11169 We will also need to account for the fact that WordPress.com does not run the latest version of the AMP plugin. It runs an old version (0.6.2 vs. the current version on W.org 1.0.2), with its own version of some of the compatibility fixes that come with the class we've added (see [[ https://[private link].php | here ]]) and needs to be updated as per the discussions here: - https://[private link] (D14521) - https://[private link] Test Plan: * Create a post with a gallery * Add a Facebook sharing button to that post. * Try Liking that post. * Comment on one of the images in the gallery. * Load the post in an AMP view, and repeat the steps below. In all cases: - You should not see any js errors in the browser console. - You should not get any PHP notices in your debug log. Reviewers: zinigor Tags: #touches_jetpack_files Differential Revision: D26821-code This commit syncs r190790-wpcom.
Summary: After adding the AMP compat file that ships with Jetpack (this was done in D22313, D26814, D26819, D26921, and D26928), let's start using this class. This replaces D22154 and D23501, and allows us to start syncing any future changes that would rely on the Jetpack_AMP_Support class. Related Jetpack PRs: - #10945 - #11195, which reverted some of the changes in the PR above. - #12054 - #12053 - #12026 Discussion: - https://[private link] We'll need to test for issues like the ones that popped up on Jetpack at the time: #11169 We will also need to account for the fact that WordPress.com does not run the latest version of the AMP plugin. It runs an old version (0.6.2 vs. the current version on W.org 1.0.2), with its own version of some of the compatibility fixes that come with the class we've added (see [[ https://[private link].php | here ]]) and needs to be updated as per the discussions here: - https://[private link] (D14521) - https://[private link] Test Plan: * Create a post with a gallery * Add a Facebook sharing button to that post. * Try Liking that post. * Comment on one of the images in the gallery. * Load the post in an AMP view, and repeat the steps below. In all cases: - You should not see any js errors in the browser console. - You should not get any PHP notices in your debug log. Reviewers: zinigor Tags: #touches_jetpack_files Differential Revision: D26821-code This commit syncs r190790-wpcom.
| 'href' => $url, | ||
| ); | ||
| if ( Jetpack_AMP_Support::is_amp_request() ) { | ||
| $menu['title'] = "<amp-img src='$img_src_2x' width=112 height=24 layout=fixed alt='$alt' title='$title'></amp-img>"; |
There was a problem hiding this comment.
This is made obsolete by #13450.
Fixes #9588
Fixes #9588
Fixes #10973
Based on #10918
Changes proposed in this Pull Request:
is_amp_endpoint()be called afterparse_query.This avoids messages like this being printed to the screen or debug.log:
It also ensures that the stats pixel is loaded
Testing instructions:
WP_DEBUGandWP_DEBUG_DISPLAYare enabledGood features to check: Carousel, Likes, Gallery, social widgets
Proposed changelog entry for your changes:
cc @westonruter