Skip to content

Conversation

justlevine
Copy link
Collaborator

@justlevine justlevine commented May 19, 2025

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 a capability 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() and tear_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 the Model, 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:

{...}

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?

  • 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.

@justlevine justlevine requested a review from Copilot May 19, 2025 18:54
Copilot

This comment was marked as outdated.

@justlevine justlevine requested a review from Copilot May 19, 2025 23:25
Copilot

This comment was marked as outdated.

@coveralls
Copy link

coveralls commented May 19, 2025

Coverage Status

coverage: 84.243% (+0.009%) from 84.234%
when pulling 774c6a8 on justlevine:dev/refactor-model
into 161565b on wp-graphql:develop.

@justlevine justlevine requested a review from Copilot May 20, 2025 09:57
Copilot

This comment was marked as outdated.

@justlevine justlevine changed the title refactor: reduce Model complexity and param types refactor: cleanup Model class to reduce complexity and improve type safety May 20, 2025
@justlevine justlevine marked this pull request as ready for review May 20, 2025 10:40
@justlevine justlevine requested a review from Copilot May 20, 2025 11:16
@justlevine justlevine added status: in review Awaiting review before merging or closing needs: reviewer response This needs the attention of a codeowner or maintainer labels May 20, 2025
@justlevine justlevine requested a review from jasonbahl May 20, 2025 11:17
Copy link
Contributor

@Copilot Copilot AI left a 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 into prepare_field() and current_user_can_access_field()
  • AbstractDataLoader introduces @phpstan-type TModel and updates return annotations to TModel

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 using call_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 ) {

Copy link

Code Climate has analyzed commit 774c6a8 and detected 0 issues on this pull request.

View more on Code Climate.

@jasonbahl jasonbahl merged commit 744003f into wp-graphql:develop Jun 2, 2025
38 checks passed
@justlevine justlevine deleted the dev/refactor-model branch June 2, 2025 20:52
justlevine pushed a commit to justlevine/wp-graphql that referenced this pull request Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: reviewer response This needs the attention of a codeowner or maintainer status: in review Awaiting review before merging or closing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants