fix: survive a second active copy, and document migrating off Composer bundling - #288
Conversation
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #288 +/- ##
============================================
- Coverage 88.17% 88.11% -0.07%
- Complexity 1259 1265 +6
============================================
Files 54 54
Lines 4120 4138 +18
============================================
+ Hits 3633 3646 +13
- Misses 487 492 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates the plugin bootstrap and autoloader behavior so a Composer-relocated, Composer-installed copy can coexist safely next to the canonical WordPress plugin without fatals or misleading “run composer install” notices, and adds migration guidance for Composer consumers.
Changes:
- Makes
mcp-adapter.phpidempotent across multiple loaded copies by inlining constant definitions and bailing early if already bootstrapped. - Updates
includes/Autoloader.phpto treat a missing localvendor/as acceptable whenWP\MCP\classes are already resolvable via another autoloader. - Adds a PHPUnit regression test and a new migration guide, with brief doc pointers from existing entry points.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/Unit/BootstrapTest.php | Adds coverage for loading a second bootstrap copy and for “autoloaded elsewhere” detection. |
| README.md | Adds pointers explaining Composer relocation behavior and links to the migration guide. |
| mcp-adapter.php | Removes the redeclare-prone constants() function; adds early guard and guarded autoloader include. |
| includes/Autoloader.php | Suppresses the missing-autoloader notice when classes are already resolvable via another autoloader. |
| docs/README.md | Adds the new migration guide to the docs index. |
| docs/migration/composer-to-plugin.md | Introduces migration guidance for Composer users, including installer-paths and bootstrap expectations. |
| docs/getting-started/installation.md | Adds a brief pointer to the migration guide for Composer installs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
From my understanding, we don't want to recommend packaging the MCP adapter in composer require, regardless of the method since support may drop in the future |
Per review on WordPress#288: bundling MCP Adapter into a consuming plugin's vendor/ should not be presented as a supported route, since support for it may be dropped. The WordPress#178 checklist item asks install docs to prioritize "usage via plugin (and wpackagist/wp-packages for Composer)". Both of those install the plugin from .org into wp-content/plugins/, which is exactly what the `wordpress-plugin` package type does. So the relocation out of vendor/ is the intended behavior, not a problem to work around. Reworked the guide accordingly: - Leads with "migrate off bundling", not with two co-equal paths. - Documents the sanctioned Composer route (wp-plugin/*, wpackagist-plugin/*) and states that the wp-content/plugins/ placement is intended. - Keeps Jetpack Autoloader and the self-bootstrap differences, but as a transitional stopgap rather than a setup guide. - Drops the `installer-paths` pin as advice; it only existed to defeat the intended install location, and now appears solely in the list of things to delete when migrating. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
matiasbenedetto
changed the title
|
Thanks @stormrockwell — you were right, and #289 made it clearer what the guide should have said. I've updated it. What changed Two things in the migration guide were still teaching the old way of doing things:
I also fixed a spot where this guide contradicted #289 about About #289 Same direction, and yours goes further — good. Two practical notes:
One thing I'd like a call on My guide documents installing via I added it because the #178 checklist says "prioritize usage via plugin (and wpackagist/wp-packages for Composer)". But those only work once we're listed on .org, so leaving them out for now is also reasonable. Happy to drop that section if you'd rather keep Composer out of the docs completely. Last thing: if it's easier to review, I can split this into a code-only PR (the crash fix, conflicts with nothing) and a docs follow-up rebased on #289. Just say the word. |
`wordpress/mcp-adapter` is published as a `wordpress-plugin`, so any project with `composer/installers` in its dependency graph gets the package relocated from `vendor/wordpress/mcp-adapter/` to `wp-content/plugins/mcp-adapter/`. That copy carries a plugin header, so it can be activated alongside the canonical plugin. Two things went wrong when it was: - WordPress loaded `mcp-adapter.php` twice from two different paths, which fatals with "Cannot redeclare WP\MCP\constants()". PHP binds unconditional top-level function declarations while the file is compiled, so a runtime guard could not have caught this — the constants are now defined inline and the file bails when `WP_MCP_DIR` is already set. - A dependency copy has no `vendor/` of its own, because Composer flattens dependencies into the root project's `vendor/`. `Autoloader::autoload()` read that as a broken install and told the user to run `composer install`, chasing a file that is not supposed to exist. It now checks whether the `WP\MCP\` classes are already resolvable through another autoloader first, and only falls back to the notice when they are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebased on WordPress#289, which removed Composer and Jetpack Autoloader from the install docs. This adds the migration half that WordPress#178 also asks for, aimed only at plugins that already bundle MCP Adapter in their own `vendor/`. - Leads with the `Requires Plugins` header, matching WordPress#289. - Walks through removing the Jetpack Autoloader, including the generated files `composer remove` leaves behind. Ordered after the bundled copy is gone, since removing it earlier lets a stale copy win against the active plugin. - Documents what a bundled copy still needs until migration finishes, without teaching how to set any of it up. - Drops the wpackagist/wp-packages install section, so this guide does not reintroduce Composer install advice that WordPress#289 deliberately removed. Also adds the CHANGELOG entries for this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
58c5d8a to
b2ce352
Compare
|
Rebased on trunk now that #289 has merged. Conflicts are gone. What I changed to fit the new docs
The guide is now purely about moving off a bundled copy. It doesn't teach installing via Composer, and it doesn't teach setting up the Jetpack Autoloader — it explains how to remove it. Check that this still holds Your verification grep from #289 passes: $ grep -rn -i "composer require\|wpackagist\|jetpack" README.md readme.txt docs/ \
| grep -v "docs/guides/testing.md" | grep -v "docs/migration/composer-to-plugin.md"
docs/troubleshooting/common-issues.md:84:# Check the autoloader exists (Jetpack Autoloader, not autoload.php)
readme.txt:72: ... (0.6.1 changelog entry)
readme.txt:93: ... (0.6.0 changelog entry)The Tests, The code changes are unchanged from before, and still don't overlap anything in #289. Offer stands to split them into their own PR if that's easier to review. |
`WP_MCP_DIR` is defined by the plugin bootstrap, so if the adapter is instantiated without it the `WP\MCP\` classes came from somewhere else — in practice a copy bundled into another plugin via `composer require wordpress/mcp-adapter`. That still runs, but the hooks, REST routes, and default server ID are global, so a bundled copy silently competes with every other copy on the site. `McpAdapter::instance()` now raises `_doing_it_wrong()` in that case, pointing developers at the `Requires Plugins` header. Visible only with `WP_DEBUG`; runtime behavior is unchanged. The check is deferred to `init` so it cannot produce false positives. Every plugin file has been loaded by then, so an active canonical plugin has certainly defined its constants and will not be mistaken for a bundled copy. It also keeps `_doing_it_wrong()` from translating its message before `init`. When the adapter is instantiated after `init` has already fired, the check runs immediately rather than hooking an action that will never run again. Verified both paths against wp-env: a normal plugin install stays silent, and loading the classes through `vendor/autoload_packages.php` with the plugin deactivated produces the notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous check only asked whether `WP_MCP_DIR` was defined. That answers "did the plugin bootstrap run", which is not the same question. The bootstrap defines the constant whether or not its own classes are the ones executing, so the check was silent in the case that matters most: the plugin is active, but Jetpack Autoloader resolved `WP\MCP\` to a newer copy bundled inside some other plugin, which then replaces the installed plugin site-wide. Verified against wp-env before the change — with the constant pointing at one directory and the class loaded from another, the old check reported canonical. It now compares where the class was actually loaded from against the plugin directory, so both situations are caught, and each gets its own message because the fix differs: - Plugin not active, classes supplied by something else. - Plugin active, but a copy elsewhere is running. The message names the file, so it is possible to tell which plugin is shipping it. Both paths are resolved with `realpath()` before comparison, since symlinked plugin directories are normal in local and Bedrock-style setups and would otherwise look like a foreign copy. The directory is compared with a trailing slash so a sibling like `mcp-adapter-fork/` cannot match on prefix alone. This also drops the old message's claim that the code came from "a copy bundled inside another plugin", which was asserting a cause that had never been established. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Added a What it doesThe MCP Adapter plugin is the canonical copy. It owns the Developer-facing only: it needs How it detects itMy first attempt just checked whether I confirmed that against wp-env before changing it. With the constant pointing at one directory and the class loaded from another, the old check reported "canonical". It now compares where the class was actually loaded from against the plugin directory, via That also let me drop the old message's claim that the code came from "a copy bundled inside another plugin" — we'd only established the bootstrap hadn't run. It could equally have been an mu-plugin or a custom loader. EvidenceFront end of a wp-env site with The plugin is not active, and something else supplied the classes.
The plugin is active, but a bundled copy is what's running. This is the case the constant check missed. Note that the message names the file, so you can tell which plugin is shipping it:
Control — plugin active, no bundled copy. Clean page, no notice: $ wp plugin activate mcp-adapter
Success: Activated 1 of 1 plugins.
$ curl -s http://localhost:8889/ | grep -c "called <strong>incorrectly"
0There's also a unit test for this.
Two things for reviewersThe version string is a guess. This is firmer than anything currently written down. #172 called bundling-through-Jetpack-Autoloader supported, and this now nags about it in every debug environment — including for a site owner running someone else's plugin that bundles a copy. It matches where #289 and this PR's review landed, but it is a policy call rather than a bug fix, so I'd rather it be reviewed as one than slip through. Happy to drop it or soften it to the inactive-plugin case only. Screenshots are hosted on an |
|
Fixed the The migration guide no longer says that WordPress installs, activates, or load-orders MCP Adapter. It now describes the actual behavior: WordPress surfaces the dependency relationship and prevents the dependent plugin from being activated while MCP Adapter is missing or inactive. The migration step also now says explicitly that |
…replaced The takeover check could not fire in the case it was written for. It lived in `McpAdapter::instance()`, but when a copy bundled inside another plugin wins the autoload race, `McpAdapter` *is* that copy — and a release older than this check has no way to report its own takeover. Verified against a real site: WordPress 6.9 with this plugin at 0.6.1 and seo-by-rank-math 1.0.276, which vendors wordpress/mcp-adapter v0.4.1 through the stock Composer autoloader and calls `McpAdapter::instance()` from its bootstrap. Today this only works because `active_plugins` happens to be alphabetical, so `mcp-adapter` loads first and wins. Reordering the option is enough to flip it: 0.4.1 then runs against the 0.6.1 plugin, and nothing reported it. The check moves to `mcp-adapter.php`, which always runs while the plugin is active no matter which copy the autoloader resolved, and is therefore the one place it can be relied on. It is self-contained on purpose: calling into the loaded classes would call a method that does not exist in whichever version won, turning a notice into a fatal on exactly the sites being warned. It is a closure for the same reason the constants are inline — a named function there would fatal on redeclaration. `McpAdapter::instance()` keeps the complementary case, where no plugin is active and something else supplied the classes. The two are mutually exclusive, so only one notice is ever raised. The message now names both versions, not just the path, since "0.4.1 loaded from seo-by-rank-math while the plugin is 0.6.1" is the whole diagnosis in one line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Tested this against a real bundler — The gapRank Math 1.0.276 bundles v0.4.1, declared include __DIR__ . '/vendor/autoload.php';
if ( class_exists( 'WP\MCP\Core\McpAdapter' ) && function_exists( 'wp_get_abilities' ) ) {
\WP\MCP\Core\McpAdapter::instance();
}With both plugins active it works — but only because Two minor versions of adapter code running against a 0.6.1 plugin, and nothing reported it. The check lived in The fixThe takeover check moves to Three things it has to respect:
The message also names both versions now, since that is the whole diagnosis in one line. EvidenceUnmodified Rank Math 1.0.276 + this branch, Rank Math loading first. No patching, no simulation — the stock plugin from the .org directory:
Default plugin order — the plugin's own copy wins, no notice: $ curl -s http://localhost:8888/ | grep -c "called <strong>incorrectly"
0That second result matters as much as the first: a real third-party bundler on a normally-ordered site produces no false positive. 1040 tests, NotesCoverage changed shape. Rather than duplicate the path logic into the bootstrap, I removed it from One inherent limit, worth stating. This cannot fire when an old bundled copy wins and the plugin is not installed, because then no current code runs at all. It protects sites that have the plugin, which is the population we can actually reach. Possibly worth raising with Rank Math. Their bundled copy is two minor versions behind and they are not using Jetpack Autoloader, so which copy wins is decided by plugin load order. Happy to open something on their side if that is useful. The earlier caveats still stand: the Screenshot hosted on an |
… running
Which copy of MCP Adapter runs is decided by autoloader registration order,
which follows plugin load order — not by anything either plugin controls. On a
site with seo-by-rank-math the installed plugin wins only because `active_plugins`
happens to be alphabetical. Deactivating and reactivating a plugin is enough to
reorder it, at which point the bundled v0.4.1 takes over silently.
Reporting only the takeover means staying quiet right up until that happens,
which is the wrong moment to start telling someone.
The bootstrap check now also looks for copies that exist but lost the race.
Registered Composer autoloaders can be asked where they *would* resolve a class
without loading it, so a dormant copy is found with no filesystem scanning:
$loader->findFile( 'WP\MCP\Core\McpAdapter' )
`findFile()` is duck-typed rather than checked against
`Composer\Autoload\ClassLoader`, so scoped and prefixed builds of Composer are
covered too.
The two cases carry different messages, since one is happening and the other
could happen, and they are mutually exclusive — a takeover returns before the
dormant scan, so at most one notice is raised per request.
Verified against stock seo-by-rank-math 1.0.276:
- default order: reports the dormant copy (previously silent)
- Rank Math first: reports the takeover, exactly one notice
- Rank Math deactivated: silent
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up: the check now also reports a bundled copy that is installed but not currently running. WhyMy last comment noted that Rank Math works today only because Reporting only the takeover means staying silent right up until the moment it happens. That is the worst possible time to start telling someone. HowRegistered Composer autoloaders can be asked where they would resolve a class, without loading it: $loader->findFile( 'WP\MCP\Core\McpAdapter' )Rank Math's EvidenceStock Rank Math 1.0.276, default plugin order — the installed plugin is running correctly, and this is the state that was silent before:
All three states against the same stock plugin:
The two messages are mutually exclusive: a takeover returns before the dormant scan runs, so the middle row emits exactly one notice, not two. I checked that explicitly rather than assuming it. 1040 tests, Two things reviewers should weighThis widens the blast radius, deliberately. Every It does not find every copy. Only copies reachable through an autoloader that is registered by The earlier caveats still stand: the Screenshot hosted on an |
Part of #178 — the "make plugin backwards-compatible with Composer Library + migration docs" item.
This takes the code and migration-guide half. The docs pass that @abhi3315 picked up (readme.txt, and restating Jetpack Autoloader as the support boundary per #172) is untouched here apart from two one-line pointers.
The problem
wordpress/mcp-adapteris published as"type": "wordpress-plugin"(#44), so Composer places it inwp-content/plugins/rather thanvendor/whenevercomposer/installersis in the dependency graph:For the plugin install method that placement is exactly right. But it also means the copy lands in a real plugin directory with a real plugin header, so it can be activated next to the canonical plugin. Two things went wrong when it was.
1. Two active copies fatal the site
WordPress loads
mcp-adapter.phptwice from two different paths:Worth being explicit about why a
defined( 'WP_MCP_DIR' ) || return;at the top of the file would not have fixed this: PHP binds unconditional top-level function declarations while the file is compiled, so the redeclaration fatals before any runtime guard executes. The constants have to move out of the named function, which is what this PR does.2. A dependency copy is told to run
composer installComposer flattens dependencies into the root project's
vendor/, so a dependency copy legitimately has novendor/of its own:Autoloader::autoload()read that as an incomplete source checkout and surfaced "The Composer autoloader was not found… make sure to runcomposer install" — pointing users at a file that is not supposed to exist. Meanwhile the classes were already loaded via the consumer's autoloader, so the state was: classes present, plugin active,Plugin::instance()never called.Both of these get more likely while people are migrating off bundled copies, not less, which is the backwards-compatibility part of the checklist item.
The changes
mcp-adapter.php— constants defined inline instead of fromconstants(), and the file bails whenWP_MCP_DIRis already defined. Therequire_onceofAutoloader.phpis guarded too, for the case where another copy'sWP\MCP\Autoloaderwas already resolved by a consumer's autoloader.includes/Autoloader.php— when there is no localvendor/autoload_packages.php, check whetherWP\MCP\is already resolvable through another autoloader before falling back to the notice. A genuine source checkout with novendor/and nothing else providing the classes still gets the notice, unchanged.docs/migration/composer-to-plugin.md— the migration guidance the checklist item asks for. Leads with migrating off a bundled copy, documents the sanctioned Composer route (wp-plugin/*,wpackagist-plugin/*) and that thewp-content/plugins/placement is intended, then keeps Jetpack Autoloader and the library-vs-plugin behavior differences (WP_MCP_DIR/WP_MCP_VERSIONundefined, dependency check skipped,wp_mcp_initnever fires,McpAdapter::instance()is yours to call) as a transitional stopgap rather than a setup guide.Tests
tests/phpunit/Unit/BootstrapTest.phpcopiesmcp-adapter.phpto a second path and requires it, which is the faithful reproduction of two installed copies. It fatals ontrunkand passes here.Full suite,
phpcs, andphpstanare all green.On not recommending Composer bundling
The first version of this guide had a "keep bundling it with Composer" section, including an
installer-pathsoverride to pin the package back intovendor/. That was wrong, and I've dropped it.The line in #178 about prioritizing "usage via plugin (and wpackagist/wp-packages for Composer)" is what settled it:
wp-plugin/*andwpackagist-plugin/*both install the plugin from .org intowp-content/plugins/, which is precisely what thewordpress-pluginpackage type exists to do. Pinning the package back intovendor/fights that. The guide now treats the relocation as a signal to migrate.I'd also read this as closing the question of whether the package type should revert to
library— it shouldn't.One thing I did not touch
README.md still has an "As a Composer Library (for plugin developers)" section that opens with "Plugin developers may wish to install MCP Adapter as a Composer dependency to integrate MCP functionality into their own plugins."
docs/getting-started/installation.mdhas the equivalent "Method 2: Composer Package". Both now read as endorsements of the thing we're steering people away from, and both should probably be reframed around installing the plugin via wpackagist/wp-packages.I only added a one-line pointer to each rather than rewriting them, since that overlaps @abhi3315's install-docs pass and seemed like a call for maintainers. Happy to take it in this PR or a follow-up.
🤖 Generated with Claude Code