-
Notifications
You must be signed in to change notification settings - Fork 463
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()
AbstractDataLoader
introduces@phpstan-type TModel
and 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
Model
class to reduce code complexity and improve maintainability and DX. More specifically:Model::$data
is 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
mixed
or a lower-fidelity type.Splits the
wrap_fields()
method into several smaller functions (Single Responsibilities principle)Fixes the
graphql_model_field_capability
filter, so it runs on all fields, not just those that are registered with acapability
property.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
Model
class have been cleaned up accordingly.Developers Note
To correctly type the
TData
generic used by theModel
, you can use the@extends
syntax on the class doc-block:For example if you are modeling data sourced from a custom
WP_Post
object:Does this close any currently open issues?
Any other comments?
AbstractDataLoader
is using a@phpstan-type
to centrally suppress the generic warning. In the future we can just swap that out with a@template
or@extends
without disrupting the rest of the file again.