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

Fix phpstan errors #3801

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Fix phpstan errors #3801

merged 1 commit into from
Jan 16, 2020

Conversation

BenMorel
Copy link
Contributor

@BenMorel BenMorel commented Dec 30, 2019

Q A
Type improvement
BC Break no
Fixed issues none

Summary

A method declared with a string return type attempts to return null. This is not caught by tests as this method is never called because SqlitePlatform does not support exceptions.

This now throws a LogicException instead, as this is not expected to be executed.

Update

Following the discussion below, this now fixes many more phpstan errors.

@BenMorel BenMorel changed the title Fix null value returned for string return type Fix phpstan errors Jan 10, 2020
@BenMorel
Copy link
Contributor Author

All phpstan errors fixed now! @morozov, waiting for your feedback on assertions, in particular.

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

@BenMorel I reviewed this partially. Please see my recommendations on using assertions and see if the rest of the assertions follow them.

lib/Doctrine/DBAL/Portability/Statement.php Show resolved Hide resolved
lib/Doctrine/DBAL/Connection.php Show resolved Hide resolved
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Platforms/SqlitePlatform.php Outdated Show resolved Hide resolved
@@ -176,6 +176,7 @@ protected function fixRow($row, bool $iterateRow, bool $fixCase)
}

if ($fixCase) {
assert($this->case !== null);
Copy link
Member

Choose a reason for hiding this comment

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

Here we could use the same approach as with private methods above, however, this one is protected. We should see what prevents this class from being finalized (#3590).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should we do here then? There are only 4 assert() left in this PR; do you want to find a solution to get rid of each of them individually, or is it OK if they stay?

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave those assertions for now. Please rebase and squash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased & squashed!

lib/Doctrine/DBAL/Query/QueryBuilder.php Show resolved Hide resolved
@morozov
Copy link
Member

morozov commented Jan 15, 2020

@BenMorel once this is done, could you please also take a look at

dbal/phpstan.neon.dist

Lines 35 to 36 in f9b3e79

- '~^Parameter #2 \$registeredAliases of static method Doctrine\\DBAL\\Query\\QueryException::unknownAlias\(\) expects array<string>, array<int, int|string> given\.\z~'
- '~^Parameter #2 \$registeredAliases of static method Doctrine\\DBAL\\Query\\QueryException::nonUniqueAlias\(\) expects array<string>, array<int, int|string> given\.\z~'

If the | is properly escaped, PHPStan produces 14 more errors that need to be fixed or properly suppressed.

@BenMorel
Copy link
Contributor Author

@BenMorel once this is done, could you please also take a look at (...)

I'll try to have a look tomorrow!

@morozov morozov merged commit e0a92b0 into doctrine:master Jan 16, 2020
@morozov
Copy link
Member

morozov commented Jan 16, 2020

Thanks @BenMorel. Great work 👍

@BenMorel BenMorel mentioned this pull request Jan 16, 2020
@morozov morozov self-assigned this Jan 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants