Check types #89
Check types #89
Conversation
garak
force-pushed
the
check-types
branch
2 times, most recently
from
51899a8
to
20214ef
Compare
|
|
||
| private function isNullable(string $property): bool | ||
| { | ||
| if (PHP_MAJOR_VERSION < 8 || !property_exists($this, $property)) { |
There was a problem hiding this comment.
Why do we need to check the PHP version here?
There was a problem hiding this comment.
Because the reflection feature is not available in PHP 7
There was a problem hiding this comment.
I see, so PHP 7 users will always have this method return false, which would result in the property not being assigned a null value, leaving it as whatever its default value is.
Would you mind adding a unit test that shows this new logic in action?
There was a problem hiding this comment.
So maybe it's better to return true in this case, so for PHP 7 the behaviour will be the same as the old one.
About testing, it should be already covered in the current test suite, the only problem I see is that the actions workflow runs only on PHP 8.0 (while it should run on every supported PHP version)
There was a problem hiding this comment.
I agree that keeping the behaviour the same as it was for platforms that don't support the new changes is a good idea
There was a problem hiding this comment.
I've added PHP 7.4 to the CI builds here: #90. You may need to rebase to pull in that change.
There was a problem hiding this comment.
I just changed the return value, squashed my commits and then rebased. I guess something has gone wrong since I see now two commits in this PR, but I'm confident that the superfluous commit will be ignored when merged
robbieaverill
force-pushed
the
check-types
branch
from
ac215c2
to
c84198b
Compare
Fix #88