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

[Config] Fix for signatures of typed properties #32466

Merged
merged 1 commit into from Jul 15, 2019

Conversation

Projects
None yet
6 participants
@tvandervorm
Copy link
Contributor

commented Jul 9, 2019

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) 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.

@tvandervorm tvandervorm changed the title Fix for signatures of public typed properties (new in PHP7.4) [Cache] Fix for signatures of public typed properties (new in PHP7.4) Jul 9, 2019

@nicolas-grekas
Copy link
Member

left a comment

(for 3.4)

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Can we please add a test? And yes, I know that that test would be skipped in our current CI setup, but still… 😅

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 10, 2019

@nicolas-grekas nicolas-grekas changed the title [Cache] Fix for signatures of public typed properties (new in PHP7.4) [Config] Fix for signatures of typed properties Jul 10, 2019

@tvandervorm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Can we please add a test? And yes, I know that that test would be skipped in our current CI setup, but still… sweat_smile

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 testHashedSignature() in ReflectionClassResourceTest, but this will cause the currently passing PHP7.1. & PHP7.3 tests to fail.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Adding a new test case annotation with @requires PHP 7.4 should, right?

@tvandervorm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

ah, I wasn't aware one could specify PHP versions in annotations like that, will give it a go, thanks

@tvandervorm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

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 testHashedSignature(), which seems ugly to me as well. Let me know what is preferred, I'll change it if needed.

Finally, I tried whether the @requires annotation gets taken into account for @dataProvider functions, which would've been the cleanest solution, this is sadly not the case however.

@tvandervorm tvandervorm force-pushed the tvandervorm:4.3 branch from 747330f to 308261a Jul 14, 2019

public function provideTypedProperties()
{
yield [1, 5, 'public array $pub;'];

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 14, 2019

Member

What if we updated the provideHashedSignature() method instead and yield these three examples only if PHP_VERSION_ID >= 70400 is true?

This comment has been minimized.

Copy link
@tvandervorm

tvandervorm Jul 14, 2019

Author Contributor

that works as well, and is a little cleaner indeed, I've updated the test.

@tvandervorm tvandervorm force-pushed the tvandervorm:4.3 branch 2 times, most recently from 5d528cf to 611bce2 Jul 14, 2019

@xabbuh

xabbuh approved these changes Jul 14, 2019

@fabpot

fabpot approved these changes Jul 15, 2019

@fabpot fabpot changed the base branch from 4.3 to 3.4 Jul 15, 2019

@fabpot fabpot force-pushed the tvandervorm:4.3 branch from 611bce2 to bad2a2c Jul 15, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Thank you @tvandervorm.

@fabpot fabpot merged commit bad2a2c into symfony:3.4 Jul 15, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jul 15, 2019

bug #32466 [Config] Fix for signatures of typed properties (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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.