Use typed properties #2473
Use typed properties #2473
Conversation
IonBazan
force-pushed
the
feature/php-7.4
branch
from
31c2d43 to
42a2059
Compare
IonBazan
force-pushed
the
feature/php-7.4
branch
from
42a2059 to
b4799a3
Compare
| * @var array<string, mixed>|mixed | ||
| */ | ||
| private $query = []; |
There was a problem hiding this comment.
This may not be an array but the actual value (when calling equals() without setting the fieldName. Reported by \Doctrine\ODM\MongoDB\Tests\Query\ExprTest::testOperatorWithoutCurrentFieldWrapsEqualityCriteria()
| @@ -12,7 +12,7 @@ | |||
| class Bucket extends AbstractBucket | |||
| { | |||
| /** @var mixed[] */ | |||
| private $boundaries; | |||
| private ?array $boundaries = null; | |||
There was a problem hiding this comment.
Why nullable? PHPDoc says it's an array
There was a problem hiding this comment.
Ah I guess it's because of null checks later. Maybe we should keep this typed as array and use isset later? On the other hand this would change how $fields = ['boundaries' => $this->boundaries]; would be behaving when no boundaries were set so maybe null is a good choice 🤔
There was a problem hiding this comment.
I fixed that by making the property private array $boundaries and using null coalesce operator below: $this->boundaries ?? null to keep the result same as before. Looks much cleaner now but Psalm complains about it.
|
I fixed the issues mentioned above. I think we need to change some phpstan rules to allow these changes if we want to avoid nullable properties. |
tests/Doctrine/ODM/MongoDB/Tests/Functional/CustomCollectionsTest.php
Outdated
Show resolved
Hide resolved
|
I haven't thought of any way to fix SA so I think that nullable optional properties will be the way to go after all. Sorry for wrong advice there 🙈 |
Summary
Makes use of PHP 7.4 typed properties where applicable. Most of it was handled by Rector 🚀 and then manually fixed up.
Additionally, I removed some invalid
@doesNotPerformAssertionsannotations on the tests that actually perform them. AddingfailOnRisky="true"inphpunit.xml.distmarks them as failed so it won't happen in the future.Linked issue #2464
While all tests are passing at the moment, it would be great if someone could help testing it in a real-life application to eliminate potential BC breaks.