Skip to content

chore: narrow/fix php types on WPGraphQL, Server, Utils namespaces #3368

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.

Sign up for GitHub

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

Jump to bottom
Merged
jasonbahl merged 5 commits into from
May 5, 2025
Merged

chore: narrow/fix php types on WPGraphQL, Server, Utils namespaces #3368

jasonbahl merged 5 commits into from
May 5, 2025
+345 −250

Conversation

@justlevine
Copy link
Collaborator

@justlevine justlevine commented Apr 26, 2025 •
edited
Loading

What does this implement/fix? Explain your changes.

This PR audits and fixes the PHPDoc types in the WPGraphQL, WPGraphQL\Server, WPGraphQL\Utils namespaces, in the following ways:

  • Replace use of mixed with a more specific type whenever possible.
    Note: Due to cross-class dependencies, numerous uses of mixed were left as is, specifically regarding the Router class.
  • Used PHPStan conditional return types to narrow types when helpful.
  • Fixed various small bugs and type errors in those and other classes as a result of the above changes.

There are no breaking changing in this PR. I've left a comment on the (limited) code-related changes.

Important

This PR is based on #3367 which should be merged first.

Relevant diff: f663eba

Does this close any currently open issues?

Any other comments?

This is a "first pass" because I really dont understand the Router class, and its reuse of the same parameters at different points in the lifecycle. Will take a second pass (in a follow up PR) once I get clarity from @jasonbahl ✔️

justlevine
justlevine commented Apr 26, 2025
View reviewed changes
justlevine
justlevine commented Apr 26, 2025
View reviewed changes
justlevine
justlevine commented Apr 26, 2025
View reviewed changes
justlevine
justlevine commented Apr 26, 2025
View reviewed changes
justlevine
justlevine commented Apr 26, 2025
View reviewed changes
justlevine
justlevine commented Apr 26, 2025
View reviewed changes
@justlevine justlevine added status: in review Awaiting review before merging or closing needs: reviewer response This needs the attention of a codeowner or maintainer type: chore Maintenance tasks, refactoring, and other non-functional changes scope: code quality Refactoring, linting, and enforcing coding standards labels Apr 26, 2025
@coveralls
Copy link

coveralls commented Apr 26, 2025 •
edited
Loading

Coverage Status

coverage: 84.27% (-0.02%) from 84.287%
when pulling f628ee9 on justlevine:chore/misc-phpstan-types
into 92713f8 on wp-graphql:develop.

@justlevine justlevine mentioned this pull request Apr 26, 2025
Merged
@justlevine justlevine changed the title chore: narrow/fix PHPDoc types on WPGraphQL and Utils namespaces chore: narrow/fix php types on WPGraphQL, Server, Utils namespaces Apr 26, 2025
jasonbahl
jasonbahl previously approved these changes Apr 28, 2025
View reviewed changes
@justlevine justlevine dismissed jasonbahl’s stale review April 28, 2025 18:58

The merge-base changed after approval.

@jasonbahl
Copy link
Collaborator

jasonbahl commented Apr 28, 2025

I really dont understand the Router class, and its reuse of the same parameters at different points in the lifecycle. Will take a second pass once I get clarity from @jasonbahl

It's been years since I touched the Router class. Can't say I understand it either 😆

Some of the inconsistencies like instantiating AppContext 2x were just symptoms of refactoring in the early days and not fully cleaning up.

@jasonbahl
Copy link
Collaborator

jasonbahl commented Apr 28, 2025

@justlevine are you looking for anything specific in regards to feedback? Changes look good to me. I'd be happy to merge now or wait for you to take another pass.

@qlty-cloud-legacy
Copy link

qlty-cloud-legacy bot commented Apr 29, 2025

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

View more on Code Climate.

@justlevine
Copy link
Collaborator Author

justlevine commented Apr 29, 2025

@justlevine are you looking for anything specific in regards to feedback? Changes look good to me. I'd be happy to merge now or wait for you to take another pass.

Nope it's ready to merge. Diff on this is already getting messy, a future pass (and perhaps some deeper cleanup) can come later.

@justlevine justlevine mentioned this pull request May 2, 2025
Merged
@jasonbahl jasonbahl merged commit dc582d7 into wp-graphql:develop May 5, 2025
40 checks passed
@jasonbahl jasonbahl mentioned this pull request May 5, 2025
Merged
@justlevine justlevine deleted the chore/misc-phpstan-types branch May 5, 2025 16:58
pull bot pushed a commit to Zezo-Ai/wp-graphql that referenced this pull request May 5, 2025
@actions-user
chore: update changeset for PR wp-graphql#3368
68e198e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonbahl jasonbahl jasonbahl left review comments

Assignees

No one assigned

Labels

needs: reviewer response This needs the attention of a codeowner or maintainer scope: code quality Refactoring, linting, and enforcing coding standards status: in review Awaiting review before merging or closing type: chore Maintenance tasks, refactoring, and other non-functional changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@justlevine @coveralls @jasonbahl