-
Notifications
You must be signed in to change notification settings - Fork 64
refactor exploration: as a wordpress plugin [WIP] #8
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
Conversation
justlevine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notes to self, probably not in a state where its worth other people chiming in yet, but if you do look at this and have thoughts please feel free to share.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't bother to rewrite the /docs for this exploration.
These steps are the dev docs.
They also have the benefit of being identical to those used in the Abilities API, so contributors dont need to juggle different project flows.
| ├── Domain/ # Business logic and MCP components | ||
| │ ├── Tools/ # MCP Tools implementation | ||
| │ │ ├── McpTool.php # Base tool class | ||
| ./includes (@todo: currently src to keep the diff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WordPress/abilities-api#3 (comment)
leaves src for assets that need to be built that arent packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these were just phpcbfed and rectored. They probably look like crap. On my todo to review before bringing this up for real discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now the standard WP PHPUnit boostrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'muplugins_loaded', | ||
| static function (): void { | ||
| // Load the abilities API, next to the MCP adapter. | ||
| require_once dirname( __DIR__, 2 ) . '/abilities-api/abilities-api.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installed by wp-env
|
Thanks for exploring this approach, @justlevine! I appreciate the thorough refactoring work here. I have some concerns about positioning this as a standalone WordPress plugin that I'd like to discuss: Core concern: In its current form, this package provides foundational infrastructure rather than end-user functionality. Making it a plugin could create confusion about its intended use case, since users installing it directly wouldn't see immediate value without additional implementation work. Potential issues I see:
Alternative approach: What if we keep this repository focused as a pure library/package, and create a separate
This approach would let us optimize each repository for its specific purpose: this one for library consumers, and a future plugin repo for end-users who need turnkey functionality. I'd love to hear your thoughts on this separation-of-concerns approach versus the unified repository model you're exploring here. |
|
@galatanovidiu thanks for diving in, even while my thoughts are still so messy 😅 . I think the broader question that needs to be answered is "what is the goal and expected use-case of the MCP Adapter?" If the plan is (still?) for this to be merged to Core - just on a longer timetable than MCP Adapter - the best practices/use cases are going to be different than if we don't have any plans for core merge (a la Two Factor, WPGraphQL, or ActionScheduler if it were .org), and even more different if we're doing upstream syncs piecemeal (like
I think the questions that need alignment are:
My understanding (per the Building Blocks post and subsequent discussions) is that we are building core MCP support for WordPress, starting "on building the server side", then possibly expanding to an MCP Client. This seems similar enough to both the WP REST API (that started as a plugin and not a Composer library) and (the canonical) WPGraphQL plugin, so even if we weren't recommending Using WPGraphQL to strengthen the example, there's also the possibility that we will need MCP-specific Admin UX down the line, even if I don't know what yet (e.g. permissions, approved clients, logs... but something that actually justifies a UI for config 😅).
(I think above answers why I don't share the
Scope creep is a risk for any project, plugin or library. The solution is defining a clear scope and adhering to it, not to avoid plugin status. AFAIK Also,this only really becomes a problem if the plan is for the MCP Adapter - plugin or library - to remain outside of Core in the very long term (2+ years) or coexist with upstream syncs, but also remember we already have a "dumping-ground" for scope creep in the form of AI Experiments.
I'm not opposed to having separate library-only and "full-fat" plugin releases, if there's a real use case for them coexisting (which to my gut seems like a weaker case than even just releasing as a library). But if so, I'd recommend we develop and release from this repo. should be just as easy to exclude the plugin wrapper files from Composer/Packagist while keeping them .zip to GH Release + .org pipleine and matching As for the reasons:
(I think this was already addressed above, if not happy to discuss more but tl;dr:) PSR-4 + namespaces should be handling all internal separation of concerns (in theory - I havn't yet gone deep into
Not entire sure I understand what this means.
As long as we follow the aforementioned "internal separation of concerns", users can still use this as a library, more or less the same way this PR (and IRL abilities api and performance) uses // If the plugin isn't installed and we dont want to use the Requires Plugins header to make them install it.
if ( ! class_exists( 'WP\MCP\Core\McpAdapter' ) {
// if we ditch Composer's autoloader for internal this wouldn't even need to plugin chrome..
// ... if that's something we want???
require_once MY_PLUGIN_PATH . '/vendor/wordpress/mcp-adapter/includes/Core/Autoload.php';
\WP\MCP\Core\Autoloader::autoload();
}
// same usage...
$adapter = WP\MCP\Core\McpAdapter::instance();But it'd be easier to propose a specific pattern for consuming this plugin as a library, if I had some insight or examples for justified use cases as to why somebody would want to go that route (and/or why we'd want them to) |
|
My opinion/understanding so far was to structure this as a composer package so others could leverage it (e.g. without needing to worry about plugin dependencies), including the AI Experiments plugin (as well as things like ClassifAI and other AI tools in the space). |
As a reminder canonical plugins can still be used as a composer packages, but composer packages require Composer and cannot be autoinstalled by WordPress. Until it's merged in core, the Abilities API is available as both a package and a plugin. @jeffpaul can you clarify
As I wrote in my last reply to @galatanovidiu , I'm hoping having a justifiable "pro-library" (versus a "yes-and") use case will help clarify the code review and other MCP Adapter objectives. |
|
The qualified feedback from this has been backported, and further iteration/feedback can happen independently. Closing. |
What
This WIP PR explores what this repo would look like as a WordPress Canonical instead of as a Composer library.
I'm also using it to explore and keep track of some of the questions I had during code review, unrelated to that primary change.
And I'll probably use this to cherry-pick some things back to core in the interim that don't conflict with the premise here or require a decision (like missing editorconfig, or the phpcs CI)
CI can be seen downstream at: justlevine#1
What's Included
srcTodo
srcresults from the rector. (e.g. the named args should be an array with a closed shape).Not now
This only makes sense if this approach gets approved in which case, all of these changes, not just the below should be done in a few separate PRs so they can be adequately reviewed.
srctoinc(ludes)- didn't want to confuse the GitHub diff.internals- I might ask some questions as they come up in the comments, but i think "how we consume an API" needs to be answered before we can weigh in on what the contract/internal behavior of an API should be.