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

Precise EntityRepository::count return type #11579

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Aug 20, 2024

Such PR was introduced on PHPStan side https://github.com/phpstan/phpstan-doctrine/pull/604/files

Either it's false, either it's true and should be directly added to the ORM code.
I already saw multiple @psalm-return 0|positive-int occurences, so I used the same syntax.

Psalm does not understand that in the BasicEntityPersister, 'SELECT COUNT(*) ... returns a positive int ; should I add the error to the baseline or typehint the result in a temporary variable ? (WDY prefer @greg0ire)

@derrabus
Copy link
Member

The static analysis failures appear to be related to your changes.

@derrabus derrabus added this to the 3.3.0 milestone Aug 20, 2024
@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Aug 20, 2024

The static analysis failures appear to be related to your changes.

Sure, it's because

return (int) $this->conn->executeQuery($sql, $params, $types)->fetchOne();

is not inferred as int<0, max but it is.

I have two ways to solve this.

  1. Writing
/** @psalm-var 0|positive-int $count */
$count = (int) $this->conn->executeQuery($sql, $params, $types)->fetchOne();

return $count;
  1. Adding the error to the baseline.

I wanted to know if you preferred one. I went with the first one. @derrabus

@greg0ire
Copy link
Member

greg0ire commented Aug 23, 2024

I think you picked the right solution, because the baseline is not made only of issues we deliberately want to ignore, but also of issues we might want to address someday ™️

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

Also, this should be targeted to 3.3.x.

greg0ire
greg0ire previously approved these changes Aug 23, 2024
@derrabus
Copy link
Member

We could also assert($count >= 0);. WDYT?

@VincentLanglet VincentLanglet force-pushed the entityRepository-count branch from cfd0886 to e648396 Compare August 23, 2024 20:49
@VincentLanglet VincentLanglet changed the base branch from 3.2.x to 3.3.x August 23, 2024 20:50
derrabus
derrabus previously approved these changes Aug 23, 2024
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

👍 once PHPCS is happy.

@VincentLanglet
Copy link
Contributor Author

👍 once PHPCS is happy.

Done

@derrabus derrabus merged commit c6b2d89 into doctrine:3.3.x Aug 24, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants