refactor: validate extensions with PHP #3246
Conversation
| * @phpstan-type Extension array{ | ||
| * name: non-empty-string, | ||
| * description: non-empty-string, | ||
| * plugin_url: non-empty-string, | ||
| * support_url: non-empty-string, | ||
| * documentation_url: non-empty-string, | ||
| * repo_url?: string, | ||
| * author: array{ | ||
| * name: non-empty-string, | ||
| * homepage?: string, | ||
| * }, | ||
| * } |
There was a problem hiding this comment.
This is all we need to validate submissions programmatically with PHPStan.
And the entire file can be dropped once we have a server payload of data with minimal changes to Extensions.php
|
|
||
| $valid_extensions = []; | ||
| foreach ( $extensions as $extension ) { | ||
| $sanitized = $this->sanitize_extension( $extension ); |
There was a problem hiding this comment.
Doing this in a separate step from validation, to reduce the tech debt of an eventual migration to a server payload.
There was a problem hiding this comment.
eventual migration to a server payload
What is this server payload?
There was a problem hiding this comment.
E.g. wp_remote_get( 'https://wpgraphql.com/{wp-json-backend-proxy}/extensions' );
If the payload of extensions is coming from outside the plugin, we need to sanitize/validate it. So I'm trying to follow conventions that would require minimal changes to that pattern switch.
| * documentation_url: non-empty-string, | ||
| * repo_url?: string, |
There was a problem hiding this comment.
These were in the documentation, but not used in the JSON schema.
Single source of truth means removing/updating them here is all we need for PHPStan to make sure that they're correctly consumed everywhere else.
There was a problem hiding this comment.
Ah yes. Good point. I did make a decision on the fly to leave them out for now as the UI doesn't use them. Figured we could make a decision on these fields later.
There was a problem hiding this comment.
- We no longer need any special instructions about looking up schemas or validation.
- I did not like the idea of having to maintain two separate sets of docs with the same info. Did my best to dedupe what's in
docs/submit-extension.md, but we can probably prune even more.
There was a problem hiding this comment.
Ok, so with proper PHP Stan types, submitting an extension will now either pass/fail the phpstan checks which nulls the Github workflow, eh?
Then as far as using the data in WPGraphQL.com, I suppose it could be as easy as localizing the json (or similar) for consumption in the NextJS site as well
|
@justlevine so, one of the reasons I went with .json was that the WPGraphQL.com website would also source the .json to populate wpgraphql.com/extensions. Since the site is built in Next it would be easy to source the .json files and use them there as well as source them in the plugin's extensions page. The .JSON could then be validated in Github to ensure extensions being added by users is valid as a merged extension will show on wpgraphql.com as well as the extensions page. The .json would also be validated at run-time in the plugin to ensure that it's valid when localizing to wp_localize_script. |
|
@jasonbahl Understood, but I do think that approach is short-sighted. Responding inline:
We can just as easily expose the extension data to GraphQL in a .com mu-plugin. Or even as a REST endpoint (even pending using .com as the source of truth), since btw the WP REST API also generates schemas and that's the pattern
PHPStan handles that validation automatically, be it publicly in WPGraphQL or in an mu-plugin sourced endpoint. And extension submitters get it in their IDE automatically without having to do any additional configs. The manual .json approach doesn't just require a new GH workflow but additional dependencies for what is a short-term solution in technical terms (from a time POV, it might take a while to serve this from .com, but I think we can agree that it doesn't make sense for the extensions source of truth to be in the plugin itself. And even if you don't, I think you can agree that creating a PR to manually update a registry - whether PHP or external .json file - is definitely not an ideal pattern and one of the first things we'd want to abstract away if the Extensions page takes off ).
This is worded like a feature, but to me it reads like a workaround for a bug: "we aren't using PHP to control the schema so we need a json validation library instead of just validating-in-code." Either way, just a recommendation |
Or dogfood GraphQL 🤦🏻 Easy enough to register a WPGraphQLExtension Type and WPGraphQLExtensionAuthor Type and expose the lists to be used on wpgraphql.com Next.js (or Astro or whatever) front end.
🤔 Like you're thinking that this will "phone home" and get a list of extensions from outside of the plugin? That wasn't my intent. I'm not 💯 opposed to that, but I wasn't looking for something to phone home to an "extensions" server or whatever. On one hand, it could make things easier to maintain a list of extensions if it is maintained outside of the codebase, but on the other hand maintaining it in the codebase is much more transparent. |
Long term I don't think anything else is really feasible or maintainable. And I think transparency is more about the guidelines, the process, and access to the list than where the list is physically stored/served from. Either way that's not a decision that needs to be made now, nor does it take away the value of doing the local sourcing in PHP vs .json, but it was definitely a factor I took into account with design. |
What does this implement/fix? Explain your changes.
Alternative approach to #3236
Rationale:
(Additionally some internals were cleaned up, but I can always leave that feedback on the original PR if this gets rejected.
Does this close any currently open issues?
Any relevant logs, error output, GraphiQL screenshots, etc?
Any other comments?
Leaving comments on the diff itself.
Where has this been tested?
Operating System: Ubuntu 24.04 (WSL + ddev + php 8.2.24)
WordPress Version: 6.7.1