-
Notifications
You must be signed in to change notification settings - Fork 178
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
Incompatibility with AMP 1.5.0 #967
Comments
@westonruter @schlessera Perhaps you could chime in here? See #968 for how it currently works. |
@swissspidy is this totally breaking things? Is this a candidate for Alpha? @pbakaus |
If someone is using AMP plugin 1.5 in Standard mode it is totally breaking things, yes. I have a workaround ready in #968 that we could use for alpha. But for beta it should be investigated more in-depth. |
yep we definitely cannot ship even an alpha without this addressed. Thanks for catching it Pascal. |
Quick fix could be for the Stories plugin to do something like: add_action( 'wp', function() {
if ( is_singular( 'web-story' ) ) {
add_filter( 'amp_enable_optimizer', '__return_false', PHP_INT_MAX );
}
} ); This should fix the problem without waiting for an AMP plugin update. |
Alternatively, which may be better, you could force the AMP plugin to not do any processing for the Stories post type. Like so: add_filter( 'amp_skip_post', function( $skipped, $post ) {
if ( 'web-story' === get_post_type( $post ) ) {
$skipped = true;
}
return $skipped;
}, PHP_INT_MAX, 2 ); This would prevent the AMP plugin from doing any processing for the Stories posts at all. |
Thanks @westonruter, I am gonna give it a try. Any recommendation for how we could still benefit from SSR in the future? |
This seems like a bug with the PHP port of the AMP Optimizer. We'll investigate. |
It appears that stories aren't supported for SSR in AMP generally, since |
Yes, indeed, Stories can't make use of SSR, meaning applying all layout tweaks on the server in order to remove the AMP boilerplate CSS (which hides the entire page content until the runtime was loaded and processed things). However, they should be able to make use of the rest of the optimizer, like inlining the runtime CSS or minification... I suspect that this current problem is not the optimizer at work, but rather the sanitizer, stripping the entire |
+1 for making sure to support Optimizer for stories for the reasons @schlessera mentioned. Also, there are more features in the pipeline where stories will massively benefit as well (e.g. module / no-module support). |
One additional note: the only feature of AMP Optimizer currently not supported by stories is splitting keyframes into a separate style block (see pr). |
Removing it out of the critical path for the alpha for now, as there seems to be a workaround. |
Just came to mind that instead of using add_filter( 'amp_enable_ssr', function( $enabled ) {
if ( is_singular( 'web-story' ) ) {
$enabled = false;
}
return $enabled;
}, PHP_INT_MAX ); |
Actually, it seems to make sense to me to use That being said, you do want to make use the AMP Optimizer. So maybe a hybrid approach is best:
|
Created #1276 to track AMP Optimizer usage |
Bug Description
Our way of rendering stories markup with output buffering clashes with the current version of the AMP plugin. There is some markup in the end, but the page is empty.
Expected Behaviour
There should be no conflicts. If anything, I should get SSR out of the box because the AMP plugin is active.
Steps to Reproduce
Install AMP plugin in Standard mode.
EXAMPLE OUTPUT - NO AMP PLUGIN
EXAMPLE OUTPUT - AMP PLUGIN INTERFERING
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance Criteria
QA Instructions
The text was updated successfully, but these errors were encountered: