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

Enforce parameter and return value types in the codebase #3348

Merged
merged 1 commit into from
May 31, 2019

Conversation

morozov
Copy link
Member

@morozov morozov commented Nov 15, 2018

Q A
Type improvement
BC Break yes
Fixed issues none

Enabled SlevomatCodingStandard.TypeHints.TypeHintDeclaration.Missing{Parameter,Return}TypeHint sniffs across the codebase.

@Majkl578
Copy link
Contributor

This looks impossible to review and properly document. Who don't we do it in smaller parts?

Also eliminating type casts/checks is probably task for separate PR (PHPStan should catch them).

@morozov
Copy link
Member Author

morozov commented Nov 18, 2018

Clean up type casting and checks like (bool) $flag or is_string($string) which are no longer needed.

Also eliminating type casts/checks is probably task for separate PR (PHPStan should catch them).

I just though about the same. Will double check if PHPStan can do that and will file a ticket otherwilse and attach it here.

@Majkl578
Copy link
Contributor

It definitely does, but probably on higher level than we have.
IMO we should've first worked on level 7 and then on enabling scalar types, this way we may end up with fatal errors in uncovered code (100% coverage is a dream).

@morozov
Copy link
Member Author

morozov commented Nov 18, 2018

This looks impossible to review and properly document. Why don't we do it in smaller parts?

I'd better try to come up with a systems approach of reviewing what's already done then splitting it apart and then putting back together. It took me a week to make this change including almost one day full time, and I may not have that much time for additional coding.

@morozov
Copy link
Member Author

morozov commented Nov 18, 2018

IMO we should've first worked on level 7 and then on enabling scalar types, this way we may end up with fatal errors in uncovered code (100% coverage is a dream).

I'd better have the branch destabilized but with all the code changes in place instead of not doing anything. AFAIK, the work on a higher PHPStan level and enabling strict types was started long ago but wasn't finished.

With this approach, we can have more non-covered breakages reported by users of develop and add more test coverage based on that.

@Majkl578
Copy link
Contributor

PHPStan level and enabling strict types was started long ago but wasn't finished.

Hmm, where? Maybe I just forgot.
Enabling strict types is separate task, not blocking scalar types or PHPStan level upgrade.

@morozov
Copy link
Member Author

morozov commented Nov 18, 2018

Hmm, where? Maybe I just forgot.

I meant #3260 and #2854.

Enabling strict types is separate task, not blocking scalar types

I see. I misread this:

IMO we should've first worked on level 7 and then on enabling scalar types

.phpunit.result.cache Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Cache/ArrayStatement.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Cache/ResultCacheStatement.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Connection.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Types/TimeImmutableType.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Types/VarDateTimeImmutableType.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Types/VarDateTimeImmutableType.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Types/VarDateTimeImmutableType.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Types/VarDateTimeType.php Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

Possibly a good idea to have #2854 in first - would catch a lot of runtime errors due to type mismatch, if there are any left

@morozov
Copy link
Member Author

morozov commented Feb 12, 2019

Yea, we discussed that internally.

@morozov morozov merged commit 593f2a5 into doctrine:develop May 31, 2019
@morozov morozov deleted the types branch May 31, 2019 14:19
@morozov morozov self-assigned this Jun 7, 2019
morozov added a commit that referenced this pull request Jun 13, 2019
Enforce parameter and return value types in the codebase
morozov added a commit that referenced this pull request Jun 27, 2019
Enforce parameter and return value types in the codebase
morozov added a commit that referenced this pull request Jun 27, 2019
Enforce parameter and return value types in the codebase
morozov added a commit that referenced this pull request Jun 27, 2019
Enforce parameter and return value types in the codebase
morozov added a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Enforce parameter and return value types in the codebase
morozov added a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Enforce parameter and return value types in the codebase
morozov added a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Enforce parameter and return value types in the codebase
morozov added a commit to morozov/dbal that referenced this pull request Aug 27, 2019
Enforce parameter and return value types in the codebase
morozov added a commit that referenced this pull request Nov 2, 2019
Enforce parameter and return value types in the codebase
@morozov morozov mentioned this pull request Apr 7, 2020
4 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 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.

5 participants