Skip to content

fix: Sabberworm dependency collision causes fatal during animation CSS parsing - #2958

Open
lucadobrescu wants to merge 4 commits into
fix/2954-autoload-missing-classfrom
fix/2942-sabberworm-collision
Open

fix: Sabberworm dependency collision causes fatal during animation CSS parsing#2958
lucadobrescu wants to merge 4 commits into
fix/2954-autoload-missing-classfrom
fix/2942-sabberworm-collision

Conversation

@lucadobrescu

Copy link
Copy Markdown
Contributor

A frontend request fataled with Declaration of CSSList::addComments(array $aComments) must be compatible with Commentable::addComments(array $comments): void while Otter parsed the animation stylesheet, because another plugin had loaded a different php-css-parser release under the same global namespace (production telemetry, Otter 3.2.1, PHP 8.1). Base_CSS::get_animation_css() now detects the foreign copy and serves the full stock stylesheet instead of parsing.

Summary

  • Base_CSS::has_own_css_parser() — new guard: three Sabberworm sentinels (Parser, Commentable, Renderable) must all resolve to files inside Otter's own vendor/. Any foreign resolution means class mixing can fatal at class-link time, and that error is not catchable, so detection must happen before the classes load.

  • Base_CSS::get_animation_css() — when the guard fails, the method enqueues the registered full otter-animation stylesheet (the same delivery the optimize-off setting uses) and returns no inline CSS. The guard runs before the otter_animations_parsed transient read because the transient stores parser objects, so even the cached path loads the colliding classes. Before → fatal at vendor/sabberworm/php-css-parser/src/CSSList/CSSList.php:474. After → the page completes and animations still work from the stock stylesheet.

  • PHPUnit regression testtests/test-animation-css.php runs the collision in a separate PHP process (tests/php/foreign-sabberworm-sandbox.php predefines the typed 9.x interface): red on the old code with the exact telemetry fatal, green now. A second test asserts the bundled parser still produces the optimized subset.

  • E2E specsabberworm-collision.spec.js (serial): the scenario mu-plugin defines the typed 9.x Commentable before plugins load, gated by the otter_e2e_foreign_sabberworm option with the REST namespace exempt so specs can always disarm it. Foreign mode → the page renders and link#otter-animation-css carries the animations; own mode → the optimized @keyframes subset is inlined and no stock stylesheet loads.

Note

Dependabot's sabberworm bump (#2890) does not resolve this: the crash comes from mixing two parser releases loaded by different plugins at runtime, which no bundled version choice can prevent. The guard is conservative — a fully foreign, self-consistent parser also triggers the fallback, because relying on another plugin's copy is not safe across major versions.

Animation CSS flow

flowchart LR
    A[Page render] --> B{Animated<br/>classes used?}
    B -- no --> Z[No animation CSS]
    B -- yes --> C{New:<br/>bundled parser<br/>resolves?}:::added
    C -- yes --> D[Parse + inline<br/>optimized subset]
    C -- no --> E[Enqueue full<br/>stock stylesheet]

    classDef added fill:#1a7f37,color:#fff,stroke:#116329,stroke-width:3px
Loading

Test instructions

  1. Create a post with a paragraph block and set Additional CSS class(es) to animated fadeIn. Publish it.

  2. Simulate a second plugin's newer parser and clear the cache:

    wp transient delete otter_animations_parsed
    cat > wp-content/mu-plugins/foreign-sabberworm.php << 'EOF'
    <?php
    namespace Sabberworm\CSS\Comment;
    interface Commentable {
        public function addComments( array $comments ): void;
        public function getComments(): array;
        public function setComments( array $comments ): void;
    }
    EOF
  3. Load the post on the frontend.

    Expect: the page renders to the end with no fatal in debug.log, and the <link id="otter-animation-css"> stylesheet is present. The paragraph animates.

  4. Remove wp-content/mu-plugins/foreign-sabberworm.php, run wp transient delete otter_animations_parsed, and reload.

    Expect: the @keyframes fadeIn rules are inlined in a <style> tag and link#otter-animation-css is gone.


Checklist before the final review

  • Included E2E or unit tests for the changes in this PR.
  • Visual elements are not affected by independent changes.
  • It is at least compatible with the minimum WordPress version.
  • It loads additional script in frontend only if it is required.
  • Does not impact the Core Web Vitals.
  • In case of deprecation, old blocks are safely migrated.
  • It is usable in Widgets and FSE.
  • Copy/Paste is working if the attributes are modified.
  • PR is following the best practices

Issue: #2942 (this PR targets the stack branch, so GitHub does not auto-close on merge — link it via the Development sidebar).

🤖 Generated with Claude Code

Base_CSS::get_animation_css() parsed the animation stylesheet with the
bundled Sabberworm parser even when another plugin had already loaded
classes from a different release of it. Mixing the two fatals at
class-link time ("Declaration of CSSList::addComments... must be
compatible") and that error is not catchable, so a guard now verifies
every sentinel class resolves to Otter's own vendor directory before
anything parser-related loads — including the transient read, which
stores parser objects. When the guard fails, the full stock
otter-animation stylesheet is enqueued instead, matching the
optimize-off delivery path.

Adds an isolated-process PHPUnit regression test and a frontend e2e
spec covering both the foreign-parser fallback and the bundled-parser
optimized path.

Fixes #2942

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@pirate-bot pirate-bot added the pr-checklist-complete The Pull Request checklist is complete. (automatic label) label Aug 3, 2026
@pirate-bot

Copy link
Copy Markdown
Contributor

Bundle Size Diff

Package Old Size New Size Diff
Animations 178.33 KB 178.33 KB 0 B (0.00%)
Blocks 1.65 MB 1.65 MB 0 B (0.00%)
CSS 7.83 KB 7.83 KB 0 B (0.00%)
Dashboard 172.49 KB 172.49 KB 0 B (0.00%)
Onboarding 68.14 KB 68.14 KB 0 B (0.00%)
Export Import 4.73 KB 4.73 KB 0 B (0.00%)
Pro 439.82 KB 439.82 KB 0 B (0.00%)

@pirate-bot

pirate-bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Plugin build for 4f78a8d is ready 🛎️!

@pirate-bot

pirate-bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

E2E Tests

Playwright Test Status: See serial and parallel matrix jobs

Performance Results serverResponse: {"q25":432.1,"q50":438.2,"q75":467.8,"cnt":10}, firstPaint: {"q25":1428.5,"q50":1469,"q75":1792.8,"cnt":10}, domContentLoaded: {"q25":3750.4,"q50":3792.5,"q75":3818.7,"cnt":10}, loaded: {"q25":3752.2,"q50":3794.3,"q75":3820.6,"cnt":10}, firstContentfulPaint: {"q25":4292.2,"q50":4328.85,"q75":4370.6,"cnt":10}, firstBlock: {"q25":14991.7,"q50":15082.15,"q75":15212.6,"cnt":10}, type: {"q25":28,"q50":29.66,"q75":32.92,"cnt":10}, typeWithoutInspector: {"q25":25.81,"q50":27.41,"q75":28.81,"cnt":10}, typeWithTopToolbar: {"q25":36.75,"q50":39.63,"q75":41.15,"cnt":10}, typeContainer: {"q25":18.43,"q50":19.81,"q75":20.33,"cnt":10}, focus: {"q25":136.45,"q50":141.91,"q75":152.8,"cnt":10}, inserterOpen: {"q25":45,"q50":47.69,"q75":50.04,"cnt":10}, inserterSearch: {"q25":16.68,"q50":18.34,"q75":20.79,"cnt":10}, inserterHover: {"q25":5.75,"q50":6.32,"q75":7.04,"cnt":20}, loadPatterns: {"q25":1872.63,"q50":1908.81,"q75":1968.22,"cnt":10}, listViewOpen: {"q25":271.76,"q50":279.43,"q75":289.37,"cnt":10}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds graceful handling for Sabberworm parser collisions during animation CSS generation.

Changes:

  • Detects foreign Sabberworm classes and falls back to the stock animation stylesheet.
  • Adds PHPUnit sandbox regression coverage.
  • Adds a serial E2E collision scenario and supporting fixtures.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
inc/class-base-css.php Adds parser ownership guard and fallback.
tests/test-animation-css.php Tests collision and bundled-parser paths.
tests/php/foreign-sabberworm-sandbox.php Reproduces the fatal in isolation.
src/blocks/test/e2e/playwright.config.js Serializes the collision spec.
src/blocks/test/e2e/fixtures.ts Adds the Sabberworm mode helper.
src/blocks/test/e2e/blocks/sabberworm-collision.spec.js Tests frontend fallback behavior.
packages/e2e-tests/mu-plugins/otter-e2e-bootstrap.php Adds scenario state and REST controls.
packages/e2e-tests/mu-plugins/includes/foreign-sabberworm-interface.php Supplies the incompatible test interface.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread inc/class-base-css.php Outdated
Comment on lines +387 to +390
if ( ! self::has_own_css_parser() ) {
// Another plugin loaded a different php-css-parser release; mixing its
// classes with the bundled ones fatals at class-link time (issue #2942).
// Skip the optimization and serve the full stock stylesheet instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 834608b — you're right, the fallback died with the cache. The delivery now lives in Blocks_Animation::frontend_load(), which runs on every request that renders an animated block regardless of the generated-CSS cache; get_animation_css() keeps only the crash guard. The e2e spec now uses a post with non-empty Otter CSS and asserts the stock stylesheet is still enqueued on a second, cache-served request.

Comment on lines +28 to +35
const post = await requestUtils.rest({
method: 'POST',
path: '/wp/v2/posts',
data: {
status: 'publish',
title,
content: POST_CONTENT
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 834608b — the spec now tracks the created post IDs and hard-deletes them in afterAll, following the best-effort pattern from dynamic-content-frontend.spec.js.

… parser collision

Copilot review: the get_animation_css() fallback enqueue was lost once
the generated post CSS was cached — later requests never reach the
parser path, and the cached CSS carries no animation rules. Move the
fallback delivery into Blocks_Animation::frontend_load(), which runs on
every request that renders an animated block, and cover a cached-CSS
reload in the e2e spec. Also delete the spec's probe posts in afterAll.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

inc/class-base-css.php:480

  • When the foreign Commentable from the reported scenario is already loaded, this loop first autoloads Otter's Parser and only rejects the foreign interface on the next iteration. That already mixes versions in the global namespace and can make the competing plugin resolve to Otter's parser later in the same request. First inspect every already-loaded sentinel with autoload disabled, and only if all loaded sentinels are owned should the method autoload missing ones.
		foreach ( $sentinels as $sentinel ) {
			if ( ! class_exists( $sentinel ) && ! interface_exists( $sentinel ) ) {
				return false;
			}

Comment thread inc/class-base-css.php
Comment on lines +471 to +475
$sentinels = array(
'\Sabberworm\CSS\Parser',
'\Sabberworm\CSS\Comment\Commentable',
'\Sabberworm\CSS\Renderable',
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 237f9a9has_own_css_parser() now scans every already-declared class, interface, and trait under Sabberworm\CSS and rejects any that does not resolve to the bundled vendor directory, before the sentinel checks can autoload anything (which also addresses the ordering concern about Parser loading ahead of the interface check). The sentinels remain only as forced entry-point resolution for the nothing-loaded-yet case. Regression coverage added: the sandbox's new outputformat scenario preloads a foreign non-sentinel class — it fatals against the sentinel-only guard and passes now.

…tinels

Copilot review: the parser touches more classes than the three sentinels
(OutputFormat, DeclarationBlock, KeyFrame, and the cached object graph),
so a foreign copy of any of them slipped past the guard and still mixed
releases. has_own_css_parser() now first rejects every already-declared
class, interface, or trait under Sabberworm\CSS that does not resolve to
the bundled vendor directory — before the sentinel checks can autoload
anything — and only then resolves the sentinel entry points. The sandbox
gains an outputformat scenario covering a foreign non-sentinel class.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

inc/class-base-css.php:479

  • PHP class and namespace names are case-insensitive, but this prefix check is case-sensitive. A preloaded foreign symbol declared as (for example) sabberworm\CSS\OutputFormat is the same runtime class as Sabberworm\CSS\OutputFormat, yet it is skipped here; the guard can then return true and the optimization can call into that foreign class. Use a case-insensitive namespace-prefix check so every colliding declaration is rejected.
			if ( 0 !== strpos( $declared_name, $prefix ) ) {

Copilot review (suppressed note): PHP class and namespace names are
case-insensitive, so a foreign symbol declared with different casing is
the same runtime class but slipped past the case-sensitive prefix scan.
Use stripos so every colliding declaration is rejected.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-checklist-complete The Pull Request checklist is complete. (automatic label)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants