AMP: load order issues

[jeherve]

Opening this to track the different issues that were caused by #10945 in Jetpack 6.9. We'll need to find a different solution to address the solution proposed in this PR, as it caused problems with different modules:

• Carousel: image meta data not added to image when image is loaded via AJAX #11151 Carousel: image meta data not added to image.
• Carousel: 400 errors on Ajax requests to get comments for each image #11148 Carousel: cannot get comments for each image.
• AMP: last AMP compatibility changes impact CSS concatenation #11123 Sharing / Multiple modules: filters are not effective anymore.

[jeherve]

cc @westonruter Moving things from init to after parse_query seems to be causing quite a few issues with different Jetpack modules. I can imagine that this could be an issue with other plugins making use of is_amp_endpoint. Has this come up since the release for you?

[westonruter]

If plugins rely on an early hook to initialize (like Jetpack), then yes, this will be something needing to be worked through.

The AMP plugin has always required the parse_query action to have fired before calling is_amp_endpoint() though during the 1.0 release cycle we did try to remove this consraint in ampproject/amp-wp#1148 to make it easier for plugins but it had to be undone. It had to be reverted because it's simply not possible to know whether it will be an AMP response until the query has been established. This is especially true when in Native mode because there is no query parameter to look for.

While is_amp_endpoint() has always required the parse_query action (and now actually wp) what has changes is using this function in regular template responses in the Native/Paired modes. So there is more surface area now.

Ultimately, the solution is to delay initialization of the modules or defer the is_amp_endpoint() checks until the wp action.

[jeherve]

Thanks, that makes sense! Do you think something like 29a3558 would be enough to solve our issues here?

[westonruter]

@jeherve The change to is_amp_request() in class.jetpack-amp-support.php I don't think is correct. By only returning a value if the wp action has happened, then it will just inexplicably return null when called and it would fail to return the right value when the request actually is AMP.

A normal refactor would look like this. Given this code:

add_action( 'init', function() {
    if ( ! is_amp_endpoint() ) {
        add_filter( 'something', 'filter_something' );
    }
} );

function filter_something( $value ) {
    $value .= 'something else';
    return $value;
}

This should be changed rather to be:

add_action( 'init', function() {
    add_filter( 'something', 'filter_something' );
} );

function filter_something( $value ) {
    if ( ! is_amp_endpoint() ) {
        $value .= 'something else';
    }
    return $value;
}

So essentially the AMP checks should be deferred to as late as possible.