Skip to content

Import core's prefixed PSR EventDispatcher in the vendored EmbeddingBuilder - #1005

Merged
dkotter merged 2 commits into
developfrom
fix/vendored-psr-prefix-guard
Sep 4, 2026
Merged

Import core's prefixed PSR EventDispatcher in the vendored EmbeddingBuilder#1005
dkotter merged 2 commits into
developfrom
fix/vendored-psr-prefix-guard

Conversation

@whyisjake

@whyisjake whyisjake commented Sep 4, 2026

Copy link
Copy Markdown
Member

The vendored EmbeddingBuilder imports an unprefixed PSR interface that does not exist under a WordPress bootstrap.

The bug

WordPress core scopes the PHP AI Client's PSR dependencies under WordPress\AiClientDependencies\. Core ships the interface at wp-includes/php-ai-client/third-party/Psr/EventDispatcher/EventDispatcherInterface.php, declaring namespace WordPress\AiClientDependencies\Psr\EventDispatcher. The vendored file imports the bare name instead:

use Psr\EventDispatcher\EventDispatcherInterface;

Why it has not blown up yet. The symbol appears only as a nullable typed property and constructor parameter:

private ?EventDispatcherInterface $eventDispatcher;
public function __construct( …, ?EventDispatcherInterface $eventDispatcher = null )

PHP does not resolve a nullable type while the value is null, and nothing currently passes a dispatcher. The first caller to pass a real one gets a TypeError — core's prefixed interface does not satisfy the unprefixed name. The failure lands when the feature is first used, not when it was introduced, which is the worst time to find it.

Not an upstream change

This is a vendoring adaptation, not a defect in php-ai-client. Upstream installs its PSR packages through Composer, where the bare name is correct — prefixing there would break their builds to fix a problem only WordPress has. There is nothing to send upstream.

The missing guard

Nothing asserted this property, so a future re-vendor could reintroduce it silently. This adds SDK_OverlayTest::test_vendored_files_use_the_prefixed_psr_dependencies(), which scans the vendored tree and fails on any unprefixed Nyholm\ or Psr\ import.

It deliberately matches every Psr\ namespace rather than Psr\Http\ alone: core prefixes Psr\EventDispatcher\ and Psr\SimpleCache\ the same way, and a PSR-7-only pattern is narrower than the hazard it exists to prevent — which is precisely how this import went unnoticed.

Testing

  • npm run test:php1571 tests, 4487 assertions, 39 skipped, 0 failures.
  • composer run-script lint — clean.
  • vendor/bin/phpstan analyse --memory-limit=3G (level 8) — no errors.

The guard was verified load-bearing rather than assumed: reverting the import to the bare name fails the new test, naming the offending file. Restored, it passes across all vendored files.

Relationship to #1004

These same changes are also present in #1004 (AI Workspace), where the guard is needed for the streaming files that PR vendors. They are byte-identical, so whichever merges first makes the other a no-op. This PR exists so the fix is not blocked behind that much larger, still-draft branch.


AI assistance: Yes. Tool(s): Claude Code (Opus 5). Used for: diagnosis, the fix, the guard test, and this description.

@whyisjake — this attestation is deliberately left for you. The WordPress AI Guidelines require the contributor to understand every line submitted and explain it under review; that is not something an agent can satisfy on your behalf. Replace this block before merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BRSJEyQyYp3XTeum8fiq3L

Open WordPress Playground Preview

…dingBuilder

WordPress core scopes the PHP AI Client's PSR dependencies under
`WordPress\AiClientDependencies\`, so the unprefixed names do not exist under a
WordPress bootstrap. The vendored `EmbeddingBuilder` imports
`Psr\EventDispatcher\EventDispatcherInterface` unprefixed, which resolves to
nothing at runtime.

It has stayed latent because the symbol appears only as a nullable typed
property and constructor parameter, and PHP does not resolve such a type while
the value is null. The first caller to pass a real dispatcher would have hit a
TypeError instead, since core's prefixed interface does not satisfy the
unprefixed name — a failure at the moment the feature was first used rather
than when it was introduced.

This is a vendoring adaptation, not an upstream defect: upstream installs its
PSR packages through Composer, where the bare name is correct, so there is
nothing to send upstream.

Nothing guarded the vendored tree against this, so a re-vendor could
reintroduce it silently. Adds a test asserting no vendored file imports an
unprefixed `Nyholm\` or `Psr\` symbol. It matches every `Psr\` namespace rather
than `Psr\Http\` alone, because core prefixes `Psr\EventDispatcher\` and
`Psr\SimpleCache\` the same way. Reverting the import turns it red.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BRSJEyQyYp3XTeum8fiq3L
@whyisjake
whyisjake requested a review from a team September 4, 2026 17:08
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: whyisjake <whyisjake@git.wordpress.org>
Co-authored-by: dkotter <dkotter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

✅ WordPress Plugin Check Report

✅ Status: Passed

📊 Report

All checks passed! No errors or warnings found.


🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.55%. Comparing base (5246098) to head (eca91ea).

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #1005   +/-   ##
==========================================
  Coverage      75.55%   75.55%           
  Complexity      3381     3381           
==========================================
  Files            138      138           
  Lines          13081    13081           
==========================================
  Hits            9883     9883           
  Misses          3198     3198           
Flag Coverage Δ
unit 75.55% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@whyisjake whyisjake self-assigned this Sep 4, 2026
@whyisjake whyisjake added this to the 1.4.0 milestone Sep 4, 2026

@dkotter dkotter 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.

Changes look correct and test well.

For future reference, we do have a specific PR template we want people to use and it doesn't appear this one uses that (seems to just be AI generated copy). That template is helpful to follow as it includes things like the changelog entry, actual test steps (not just automated tests) and AI usage.

@dkotter
dkotter merged commit a7657e1 into develop Sep 4, 2026
59 of 60 checks passed
@dkotter
dkotter deleted the fix/vendored-psr-prefix-guard branch September 4, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants