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.

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

Mark nullable EntityRepository methods as impure #648

Open
wants to merge 1 commit into
base: 2.0.x
Choose a base branch
from
Open

Mark nullable EntityRepository methods as impure #648

wants to merge 1 commit into from

Conversation

nCrazed
Copy link

@nCrazed nCrazed commented Mar 13, 2025

Resolves #550

@nCrazed
Mark nullable EntityRepository methods as impure
a1d55bb 
The result of these functions depends on the underlying database state
which can change between multiple invocations. For example:

Calling find() or findOneBy() and asserting the result to be null as a
precondition of a test will generate false errors later in the test when the
underlying record is added and the method is called again

See phpstan#550
@nCrazed
Copy link
Author

nCrazed commented Mar 13, 2025

At a glance, all of the failures seem to be due to missing Microsoft ODBC Driver which is completely unrelated to the changes in this PR

cs278
cs278 reviewed Mar 14, 2025
View reviewed changes
Copy link

@cs278 cs278 left a comment

Choose a reason for hiding this comment

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

Should findAll(), findBy(), matching(), createQueryBuilder() and count() also be marked as impure?

@nCrazed
Copy link
Author

nCrazed commented Mar 14, 2025 •
edited
Loading

They don't return a union type so them being pure isn't a bug in the context of type checking

Unless PHPStan has some functionality that relies on caching actual values returned (as opposed to just their type), I don't think it's worth being that pedantic about it.

@cs278
Copy link

cs278 commented Mar 17, 2025

Unless PHPStan has some functionality that relies on caching actual values returned (as opposed to just their type), I don't think it's worth being that pedantic about it.

PHPStan's type system is more descriptive than PHP's: https://phpstan.org/r/ab040ba5-b25a-465e-8c65-3560fa1b20b3

Pure function always returns the same value if its inputs (object state and arguments) are the same, and has no side effects. Impure function has side effects and its return value might change even if the input does not.
https://phpstan.org/blog/remembering-and-forgetting-returned-values

@nCrazed
Copy link
Author

nCrazed commented Mar 17, 2025

This PR aims to address a specific bug that is actively breaking builds that, to the best of my knowledge, does not affect the methods you've highlighted.

Unless @ondrejmirtes chimes in with desire for those methods to also be changed in this PR I am leaving it as is.

Any larger efforts to address semantic correctness of the stubs in this projects can be taken care off in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cs278 cs278 cs278 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Repository methods considered pure
2 participants
@nCrazed @cs278