[Config] Fix for signatures of typed properties #32466
Conversation
|
Can we please add a test? And yes, I know that that test would be skipped in our current CI setup, but still… 😅 |
I'd be happy to write a testcase for this issue, but it's not clear to me how I could do this without breaking the test for PHP < 7.4 . The most straightforward way would be to add a line to |
|
Adding a new test case annotation with |
|
ah, I wasn't aware one could specify PHP versions in annotations like that, will give it a go, thanks |
|
I added a little testcase, which I tested with both PHP7.2 & PHP 7.4. It gets skipped in the first case, passes in the second case and fails in the second case if I revert my change, so it does what it should do. Now, I've always been taught that dependencies between tests should be avoided, which I failed to do here. However, the alternative would be to copy/paste the content of Finally, I tried whether the |
|
|
||
| public function provideTypedProperties() | ||
| { | ||
| yield [1, 5, 'public array $pub;']; |
There was a problem hiding this comment.
What if we updated the provideHashedSignature() method instead and yield these three examples only if PHP_VERSION_ID >= 70400 is true?
There was a problem hiding this comment.
that works as well, and is a little cleaner indeed, I've updated the test.
5d528cf to
611bce2
Compare
|
Thank you @tvandervorm. |
This PR was submitted for the 4.3 branch but it was squashed and merged into the 3.4 branch instead (closes #32466). Discussion ---------- [Config] Fix for signatures of typed properties | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #32465 | License | MIT | Doc PR | - Also see the issue description, when using public typed properties ([new in PHP7.4](https://wiki.php.net/rfc/typed_properties_v2)) like this: ``` namespace App; class Foo { public int $bar; } ``` will cause `$defaults['bar']` not to be set in Symfony/Component/Config/Resource/ReflectionClassResource.php::139. This is because `$bar` doesn't have a default value, but does have a type hint, meaning it's default value is not `null` but undefined. This causes an 'undefined index' error when clearing the cache through `bin/console cache:clear` when running PHP7.4. The default value is used here for the class signature, having `null` should be appropriate for all cases. Commits ------- bad2a2c [Config] Fix for signatures of typed properties
Also see the issue description, when using public typed properties (new in PHP7.4) like this:
will cause
$defaults['bar']not to be set in Symfony/Component/Config/Resource/ReflectionClassResource.php::139.This is because
$bardoesn't have a default value, but does have a type hint, meaning it's default value is notnullbut undefined. This causes an 'undefined index' error when clearing the cache throughbin/console cache:clearwhen running PHP7.4.The default value is used here for the class signature, having
nullshould be appropriate for all cases.