Skip to content
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

refactor: make AbstractConnectionResolver::should_execute() non-abstract and add ::pre_should_execute() #3104

Conversation

justlevine
Copy link
Collaborator

What does this implement/fix? Explain your changes.

This PR refactors the DX around AbstractConnectionResolver::should_execute().

More specifically:

  • AbstractConnectionResolver::$should_execute is no longer instantiated with a value. Instead ::should_execute() is no longer abstract and returns true by default, so only Resolvers that require custom should_execute logic need to overload the method.
  • Resolver-specific pre_should_execute logic has been abstracted out of AbstractConnectionResolver's constructor and into a new ::pre_should_execute() method. This overloadable method is then wrapped in a new ::get_pre_should_execute() method which uniformly filters the value with the new graphql_connection_pre_should_execute hook.
  • ::get_should_execute() has been modified to call ::should_execute() if the class property has yet to be set. This ensure backwards compatibility and paves the way for future enhancements, such as improving the evaluation logic and filtering in ::execute_and_get_ids().

There are no breaking changes in this PR.

Does this close any currently open issues?

Part of #2749

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

  • Due to the way these changes are implemented, this PR is fully backwards compatible. Existing Resolvers will continue to work as before, and developers will be able to update their classes at their leisure to "opt in" to the new features like moving their constructor pre-checks into get_pre_should_execute(), separating the pre/should_execute logic, and removing unnecessary parameter instantiation, method overloading.

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.1.15)

WordPress Version: 6.5.2

@justlevine justlevine changed the title refactor: make AbstractConnectionResolver::should_execute() non-abstract and add pre_should_execute() refactor: make AbstractConnectionResolver::should_execute() non-abstract and add ::pre_should_execute() Apr 24, 2024
@justlevine justlevine added Type: Enhancement New feature or request Status: In Review Needs to be reviewed by a project maintainer before merge Needs: Reviewer Response This needs the attention of a codeowner or maintainer. Component: Connections Issues related to connections scope: API Issues related to access functions, actions, and filters labels Apr 24, 2024
@coveralls
Copy link

coveralls commented Apr 24, 2024

Coverage Status

coverage: 84.275% (-0.01%) from 84.289%
when pulling 8014e2c on justlevine:dev/cr2-backport/refactor-should_execute
into a66f3e1 on wp-graphql:develop.

Copy link
Collaborator

@jasonbahl jasonbahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment regarding a @todo comment

src/Data/Connection/AbstractConnectionResolver.php Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Apr 25, 2024

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

View more on Code Climate.

@jasonbahl jasonbahl merged commit 56fbf54 into wp-graphql:develop May 2, 2024
29 of 30 checks passed
@jasonbahl jasonbahl mentioned this pull request May 2, 2024
@justlevine justlevine deleted the dev/cr2-backport/refactor-should_execute branch May 3, 2024 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Connections Issues related to connections Needs: Reviewer Response This needs the attention of a codeowner or maintainer. scope: API Issues related to access functions, actions, and filters Status: In Review Needs to be reviewed by a project maintainer before merge Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants