Skip to content

[PropertyInfo] Fix typed collections in PHP 7.4 #38041

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Sep 4, 2020
Merged

[PropertyInfo] Fix typed collections in PHP 7.4 #38041

fabpot merged 1 commit into from
Sep 4, 2020

Conversation

@ndench
Copy link
Contributor

@ndench ndench commented Sep 3, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets No ticket
License MIT
Doc PR N/A

#37971 introduced support for typed properties in PHP 7.4, however by short circuiting the getTypes() method, typed collections are returned as Type::BUILTIN_TYPE_ARRAY without a proper collection type. If running on PHP < 7.4, the getMutator() method would be called which would extract the collection type from the getter/setter or adder/remover.

I updated the typedPropertiesTest to cover this case.

@ndench ndench requested a review from dunglas as a code owner September 3, 2020 00:33
@ndench
Copy link
Contributor Author

ndench commented Sep 3, 2020

Travis is failing on FrameworkBundle tests, it appears to be unrelated to this PR as the same test failures are present in a number of other open PRs. If my PR has somehow affected these tests I'm happy to fix them if someone can point in the right direction.

Test failures 
There were 3 failures:

1) Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\PhpFrameworkExtensionTest::testCachePoolServices
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'bindings' => Array ()
             'errors' => Array ()
             'arguments' => Array (
-                'index_0' => 'xctxZ1lyiH'
+                'index_0' => 'UKoP1K+Hox'
                 'index_1' => 12
             )
             'innerServiceId' => null
@@ @@
             'errors' => Array ()
             'arguments' => Array (
                 'index_0' => Symfony\Component\DependencyInjection\Reference Object (...)
-                'index_1' => 'xctxZ1lyiH'
+                'index_1' => 'UKoP1K+Hox'
                 'index_2' => 12
             )
             'innerServiceId' => null
             
/home/travis/build/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php:1476

2) Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\XmlFrameworkExtensionTest::testCachePoolServices
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'bindings' => Array ()
             'errors' => Array ()
             'arguments' => Array (
-                'index_0' => 'xctxZ1lyiH'
+                'index_0' => 'UKoP1K+Hox'
                 'index_1' => 12
             )
             'innerServiceId' => null
@@ @@
             'errors' => Array ()
             'arguments' => Array (
                 'index_0' => Symfony\Component\DependencyInjection\Reference Object (...)
-                'index_1' => 'xctxZ1lyiH'
+                'index_1' => 'UKoP1K+Hox'
                 'index_2' => 12
             )
             'innerServiceId' => null
             
/home/travis/build/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php:1476

3) Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\YamlFrameworkExtensionTest::testCachePoolServices
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
             'bindings' => Array ()
             'errors' => Array ()
             'arguments' => Array (
-                'index_0' => 'xctxZ1lyiH'
+                'index_0' => 'UKoP1K+Hox'
                 'index_1' => 12
             )
             'innerServiceId' => null
@@ @@
             'errors' => Array ()
             'arguments' => Array (
                 'index_0' => Symfony\Component\DependencyInjection\Reference Object (...)
-                'index_1' => 'xctxZ1lyiH'
+                'index_1' => 'UKoP1K+Hox'
                 'index_2' => 12
             )
             'innerServiceId' => null
             
/home/travis/build/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php:1476

FAILURES!
Tests: 873, Assertions: 2537, Failures: 3, Skipped: 5.

@fabpot
Copy link
Member

fabpot commented Sep 3, 2020

/cc @ogizanagi

ogizanagi
ogizanagi approved these changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me appart from my comment perhaps. As I'm not the original author, I may miss something, though.

Thank you for the PR!

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
return $fromDefaultValue;
}

if (\PHP_VERSION_ID >= 70400) {
Copy link
Contributor

@ogizanagi ogizanagi Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll make more sense to have precedence over constructor and default value extraction (the two if above), right?

Copy link
Contributor Author

@ndench ndench Sep 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ogizanagi yeah it might make more sense in the fact that if there is constructor args or default values as well as a php7.4 property type it might be more simple to just extract from the property type? I'm not sure it makes much of a difference though. Happy to move it if you like?

Copy link
Contributor

@rogamoore rogamoore Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like moving this block has been changing the previous behaviour quite a bit.
#38416

Maybe you could have a look, if this could be reverted (while still fixing #38041)

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Sep 3, 2020
fabpot
fabpot approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I slightly prefer like this (regarding the comment)

@fabpot
Copy link
Member

fabpot commented Sep 4, 2020

Thank you @ndench.

@fabpot fabpot merged commit 4482fcf into symfony:4.4 Sep 4, 2020
@ndench ndench deleted the property-info-array-type branch September 9, 2020 21:38
This was referenced Sep 27, 2020
Merged
Merged
@odolbeau odolbeau mentioned this pull request Oct 5, 2020
Closed
@rogamoore rogamoore mentioned this pull request Oct 5, 2020
Closed
This was referenced Oct 6, 2020
Closed
Merged
This was referenced Apr 3, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

+2 more reviewers

@rogamoore rogamoore rogamoore left review comments

@ogizanagi ogizanagi ogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug PropertyInfo Status: Reviewed

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

6 participants

@ndench @fabpot @ogizanagi @rogamoore @nicolas-grekas @carsonbot