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

Remove static from GraphQL\Validator\Rules\ValuesOfCorrectType #851

Closed
wimski opened this issue Apr 26, 2021 · 3 comments · Fixed by #854
Closed

Remove static from GraphQL\Validator\Rules\ValuesOfCorrectType #851

wimski opened this issue Apr 26, 2021 · 3 comments · Fixed by #854
Labels

Comments

@wimski
Copy link
Contributor

wimski commented Apr 26, 2021

A lot of static methods are used in ValuesOfCorrectType, but when I take a closer look I can't see why. There seems to be no added benefit in having these static. Would it be possible to remove all the static usage from this class?

The reason I ask is because I want to extend it in order to change the way some error messages are created. This feels related to #711, but I want to do more than a simple string replacement.

@spawnia
Copy link
Collaborator

spawnia commented Apr 26, 2021

There seems to be no added benefit in having these static.

Those methods can be called without class instantiation, this is done from tests. How would removing static improve extensibility?

I think the deciciding difference is in how those methods are called. self::method() prevents overrides, while static::method() works.

@wimski
Copy link
Contributor Author

wimski commented Apr 26, 2021

Those methods can be called without class instantiation, this is done from tests.

So the only reason they are static is because of tests?

I think the deciciding difference is in how those methods are called. self::method() prevents overrides, while static::method() works.

Then replacing self with static would also work for my case.

@spawnia
Copy link
Collaborator

spawnia commented Apr 26, 2021

So the only reason they are static is because of tests?

I just named one reason, there may be many.

Another one I can think of right now is that static methods are generally easier to reason about; one does (generally) not have to worry about state with them.

Then replacing self with static would also work for my case.

Yes. Accepting PRs, but preferrably one that opens up all validation rules for extensibility to ensure consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants