Skip to content

Fix indexBy type extraction #20051

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

Merged
merged 1 commit into from
Sep 28, 2016
Merged

Conversation

lemoinem
Copy link
Contributor

@lemoinem lemoinem commented Sep 24, 2016

Q A
Branch? 2.8+
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

Bug found and patched by @ksom

Since 3008228, the Doctrine Bridge's PropertyInfo Extractor tries to extract the type of the key in an indexed association. However, as you can see in 3008228#diff-7a8fb8072d57f95ea6e37898b05895bcR91, the extractor was using the metadata of the class containing the association instead of the target entity's metadata to retrieve the type of the index.

The tests were green because in 3008228#diff-c7e914ed89ceffd939733efe08c039c2R44, the property used to indexBy was present in the classes on both sides of the relation with the same type.

Once the test is fixed (by renaming the property in the targetEntity), the test provided at 3008228#diff-1b2e044d1df011c00caf802a07895bdbR88 gives the error

1) Symfony\Bridge\Doctrine\PropertyInfo\Tests\DoctrineExtractorTest::testExtract with data set #9 ('indexedBar', array(Symfony\Component\PropertyInfo\Type Object (...)))
InvalidArgumentException: "" is not a valid PHP type.

The fix is to fetch the metadata of the target entity and check there for the property type.

@lemoinem lemoinem changed the base branch from master to 2.8 September 24, 2016 21:44
@lemoinem
Copy link
Contributor Author

The travis build failed because of an issue with the LDAP test (src/Symfony/Component/Ldap/Tests/Fixtures/data/base.ldif: No such file or directory) which occurs in the HHVM environment.

@ksom
Copy link

ksom commented Sep 25, 2016

@lemoinem Thanks for mentioning me :)

@fabpot
Copy link
Member

fabpot commented Sep 28, 2016

Thank you @lemoinem.

@fabpot fabpot merged commit 138c6e3 into symfony:2.8 Sep 28, 2016
fabpot added a commit that referenced this pull request Sep 28, 2016
This PR was merged into the 2.8 branch.

Discussion
----------

Fix indexBy type extraction

| Q             | A
| ------------- | ---
| Branch?       | 2.8+
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | N/A

Bug found and patched by @ksom

Since 3008228, the Doctrine Bridge's PropertyInfo Extractor tries to extract the type of the key in an indexed association. However, as you can see in 3008228#diff-7a8fb8072d57f95ea6e37898b05895bcR91, the extractor was using the metadata of the class containing the association instead of the target entity's metadata to retrieve the type of the index.

The tests were green because in 3008228#diff-c7e914ed89ceffd939733efe08c039c2R44, the property used to `indexBy` was present in the classes on both sides of the relation with the same type.

Once the test is fixed (by renaming the property in the targetEntity), the test provided at 3008228#diff-1b2e044d1df011c00caf802a07895bdbR88 gives the error

    1) Symfony\Bridge\Doctrine\PropertyInfo\Tests\DoctrineExtractorTest::testExtract with data set #9 ('indexedBar', array(Symfony\Component\PropertyInfo\Type Object (...)))
    InvalidArgumentException: "" is not a valid PHP type.

The fix is to fetch the metadata of the target entity and check there for the property type.

Commits
-------

138c6e3 Fix indexBy type extraction
@lemoinem lemoinem deleted the fix/index-by-type-extraction branch September 28, 2016 02:21
This was referenced Oct 3, 2016
@teohhanhui
Copy link
Contributor

Oops! Thanks for catching this. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants