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

Gracefully handle conflicting version of PHP-CSS-Parser being loaded #1743

Merged
merged 1 commit into from Dec 14, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

commented Dec 14, 2018

Via https://wordpress.org/support/topic/500-error-on-post-pages-without-yoast-glue-plugin/

When another plugin/theme is installed which also has a copy of PHP-CSS-Parser, if its Composer autoloader wins then the required methods and functionality of our fork will not be available. In this case, all of the CSS processing will not be available, including tree shaking.

So this PR detects for a conflicting version being installed, and it will show a warning:

image

Additionally, feature detection is now done with fallbacks to the methods that are available.

When the required PHP-CSS-Parser is not available, then the style[amp-custom] manifest comment will indicate this with a warning:

The style[amp-custom] element is populated with:
     0 B: style[amp-custom=]
    81 B: style
  1031 B: link#wp-block-library-theme-css[rel=stylesheet][id=wp-block-library-theme-css][href=https://src.wordpress-develop.test/wp-content/plugins/gutenberg/build/block-library/theme.css?ver=1543961680][type=text/css][media=all]
 26102 B: link#wp-block-library-css[rel=stylesheet][id=wp-block-library-css][href=https://src.wordpress-develop.test/wp-content/plugins/gutenberg/build/block-library/style.css?ver=1543961680][type=text/css][media=all]
   454 B: style#twentynineteen-style-inline-css[id=twentynineteen-style-inline-css][type=text/css]
  3040 B: link#twentynineteen-print-style-css[rel=stylesheet][id=twentynineteen-print-style-css][href=https://src.wordpress-develop.test/wp-content/themes/twentynineteen/print.css?ver=1.0][type=text/css][media=print]
   248 B: link#amp-default-css[rel=stylesheet][id=amp-default-css][href=https://src.wordpress-develop.test/wp-content/plugins/amp/assets/css/amp-default.css?ver=1.0.1][type=text/css][media=all]
   122 B: style[type=text/css]
   168 B: span.amp-wp-da211f7[class=amp-wp-da211f7]
    74 B: figure.wp-caption alignnone amp-wp-00fd489[class=wp-caption alignnone amp-wp-00fd489]
    89 B: amp-img.image wp-image-580 foo bar attachment-full size-full amp-wp-enforced-sizes amp-wp-36746cb[width=652][height=96][src=https://src.wordpress-develop.test/wp-content/uploads/2018/02/Grouplogo-1.png][class=image wp-image-580 foo bar attachment-full size-full amp-wp-enforced-sizes amp-wp-36746cb][alt=Example alt value][title=example image title][srcset=https://src.wordpress-develop.test/wp-content/uploads/2018/02/Grouplogo-1.png 652w, https://src.wordpress-develop.test/wp-content/uploads/2018/02/Grouplogo-1-300x44.png 300w][sizes=(max-width: 652px) 100vw, 652px][layout=intrinsic]
Total included size: 31,409 bytes

The following stylesheets are too large to be included in style[amp-custom]:
 87064 B: link#twentynineteen-style-css[rel=stylesheet][id=twentynineteen-style-css][href=https://src.wordpress-develop.test/wp-content/themes/twentynineteen/style.css?ver=1.0][type=text/css][media=all]
Total excluded size: 87,064 bytes

!!!WARNING!!! AMP CSS processing is limited because a conflicting version of PHP-CSS-Parser has been loaded by another plugin/theme. Tree shaking is not available.

Classic mode in particular should work acceptably when the required version of PHP-CSS-Parser is not available.

@westonruter westonruter added this to the v 1.0.2 milestone Dec 14, 2018

@westonruter westonruter requested a review from kienstra Dec 14, 2018

@googlebot googlebot added the cla: yes label Dec 14, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

Here's a build of the plugin on this branch: amp.zip

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2018

Approved

Hi @westonruter,
This looks good.

The admin notice appeared when the Mailpoet plugin was active, but it didn't appear otherwise.

@@ -1946,6 +2006,10 @@ private function finalize_styles() {
}
}
if ( ! self::has_required_php_css_parser() ) {
$comment .= "\n" . esc_html__( '!!!WARNING!!! AMP CSS processing is limited because a conflicting version of PHP-CSS-Parser has been loaded by another plugin/theme. Tree shaking is not available.', 'amp' ) . "\n";
}

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 14, 2018

Collaborator

This looks good when the Mailpoet plugin is active.

warning-appears

'<div class="notice notice-warning"><p>%s</p></div>',
sprintf(
/* translators: %s is location where conflicting lib was found */
esc_html__( "A conflicting version of PHP-CSS-Parser appears to be installed by another plugin/theme (located in '%s'). Because of this CSS processing will be limited, and tree shaking will not be available.", 'amp' ),

This comment has been minimized.

Copy link
@kienstra

kienstra Dec 14, 2018

Collaborator

This notice looks good when Mailpoet is active:

notice-mailpoet-active

@westonruter westonruter changed the base branch from develop to 1.0 Dec 14, 2018

@westonruter westonruter merged commit 94f87e8 into 1.0 Dec 14, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/php-css-parser-dependency-checks branch Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.