-
Notifications
You must be signed in to change notification settings - Fork 466
refactor: cleanup Model class to reduce complexity and improve type safety
#3381
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
Model class to reduce complexity and improve type safety
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.
Pull Request Overview
This PR refactors the core Model class to leverage generics for stronger type safety, breaks down a large field-wrapping method into smaller responsibilities, and updates data loader return types to use a PHPStan generic alias.
- Model subclasses now declare their data type via
@extends Model<…>instead of redefining$data Model::wrap_fields()was split intoprepare_field()andcurrent_user_can_access_field()AbstractDataLoaderintroduces@phpstan-type TModeland updates return annotations toTModel
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Model/UserRole.php | Replaced inline $data docblock with @extends Model<…> |
| src/Model/User.php | Updated class-level docblock to use generic @extends |
| src/Model/Theme.php | Switched $data docblock to a generic @extends |
| src/Model/Term.php | Added @extends Model<\WP_Term> and removed $data property |
| src/Model/Taxonomy.php | Updated docblock to use generic @extends |
| src/Model/PostType.php | Replaced $data property doc with @extends |
| src/Model/Post.php | Added @extends Model<\WP_Post> and removed redundant $data |
| src/Model/Plugin.php | Swapped $data doc for @extends Model<array<string,mixed>> |
| src/Model/Model.php | Refactored __get, split wrap_fields(), added prepare_field() and current_user_can_access_field() |
| src/Model/MenuItem.php | Updated class doc to generic @extends |
| src/Model/Menu.php | Migrated $data doc to @extends Model<\WP_Term> |
| src/Model/CommentAuthor.php | Declared @extends Model<\WP_Comment> in class docblock |
| src/Model/Comment.php | Updated docblock to use generic @extends |
| src/Model/Avatar.php | Replaced $data doc with @extends Model<array<string,mixed>> |
| src/Data/Loader/AbstractDataLoader.php | Introduced @phpstan-type TModel and changed return annotations to ?TModel |
Comments suppressed due to low confidence (2)
src/Model/Model.php:147
- For better performance and readability, you can invoke the callable directly (e.g.,
$data = ($this->fields[$key])();) instead of usingcall_user_func.
$data = call_user_func( $this->fields[ $key ] );
src/Model/Model.php:390
- The new
prepare_field()logic (including the handling of callbacks and permission checks) should be covered by unit tests to verify both callable and non-callable scenarios, as well as filter overrides.
private function prepare_field( string $field_name, $field ) {
|
Code Climate has analyzed commit 774c6a8 and detected 0 issues on this pull request. View more on Code Climate. |
What does this implement/fix? Explain your changes.
This PR refactors the
Modelclass to reduce code complexity and improve maintainability and DX. More specifically:Model::$datais now typed via a Generic type, instead of needlessly type-hinting in child classes.As a result, many doc types have been updated to use that generic instead of
mixedor a lower-fidelity type.Splits the
wrap_fields()method into several smaller functions (Single Responsibilities principle)Fixes the
graphql_model_field_capabilityfilter, so it runs on all fields, not just those that are registered with acapabilityproperty.This allows the filter to be used to add capability checks to existing fields, and not just to modify preset capabilities.
Clarifies usage of various functions, specifically that
setup()andtear_down()functions apply per field.Files that rely on the
Modelclass have been cleaned up accordingly.Developers Note
To correctly type the
TDatageneric used by theModel, you can use the@extendssyntax on the class doc-block:For example if you are modeling data sourced from a custom
WP_Postobject:{...} use \WPGraphQL\Model\Model; /** * {other class-docblock meta} * * @extends \WPGraphQL\Model\Model<\WP_Post> */ class EventCptModel extends Model { ... }Does this close any currently open issues?
Any other comments?
AbstractDataLoaderis using a@phpstan-typeto centrally suppress the generic warning. In the future we can just swap that out with a@templateor@extendswithout disrupting the rest of the file again.