Add failing test for issue #7062 - BasicEntityPersister confuses association types with PDO datatypes #7082
Conversation
|
@mariusklocke maybe you intended to send this to |
|
Changed to 2.6 |
There was a problem hiding this comment.
@mariusklocke can you please combine the models in the test file just like https://github.com/doctrine/doctrine2/blob/30a063ef9d7b14d0c74a192d87dc31c600503a4b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6531Test.php ?
This would also make things easier for us to review, you to fix coding standard issues using phpcbf, and also removes all the changes you've made in OrmFunctionalTestCase (which are not really necessary).
| // Now try to read entities again and check if all changes have been made persistent correctly | ||
| /** @var Season $season */ | ||
| $season = $this->_em->find(Season::class, self::SEASON_ID); | ||
| $this->assertInstanceOf(Season::class, $season); |
There was a problem hiding this comment.
Please use self::assert* instead since the method is static.
|
Thanks for your quick feedback. Will do that in the next days. |
|
@mariusklocke @Ocramius wow, this is a tough one! The cause of the issue is that for some reason the ORM is not using the correct type for the I just now realised that the whole thing was already described in the issue, stupid me 😂 |
There was a problem hiding this comment.
@mariusklocke I've changed things a bit in the test to always fetch things from the database, just to ensure that things are isolated.
The fix itself was pretty simple (with a little help from @guilhermeblanco).
Thanks for your contribution!
@mariusklocke I read it, after I've posted my comment... you're assumptions are indeed correct |
|
I'm glad you found a fix. Debugging that took me a while :) |
|
@mariusklocke the only thing is that your mapping quite complex and it uncovers some scenarios that we haven't thought about (the fact that |
| /** | ||
| * @Id | ||
| * @OneToOne(targetEntity=GH7062Season::class, inversedBy="ranking") | ||
| * @JoinColumn(name="season", referencedColumnName="id") |
There was a problem hiding this comment.
@Ocramius @guilhermeblanco this test revealed a nice unrelated issue (when loading things with L2C the identifier flattener tries to use the column name in an array that doesn't have that index).
Forcing the entity to use the name of the field as the name of the column was the only way to isolate the issue that @mariusklocke described... what are your thoughts about this workaround?
There was a problem hiding this comment.
@lcobucci as long as we can open a new issue with the new problem, I'm good to isolate issues in 2 separate PRs. =)
There was a problem hiding this comment.
@guilhermeblanco that's the idea
There was a problem hiding this comment.
Yeah, that's a separate issue - let's not creep it into here.
There was a problem hiding this comment.
I'm fine with the patch overall, but the test uses a DSL from a different domain, and that makes things quite complex to follow when reasoning about ORM details.
I'd rather get rid of all the public methods in these entities, and modify all states via public properties instead, making the classes ORM-naming-specific instead of flavored with Season and Team
Which was using the wrong way to fetch the field type and using the association type instead of the column type.
|
@Ocramius how about now? |
|
@lcobucci if the test still fails with the patch not applied, then 👍 I'd rather completely get rid of the team/season/etc thing... |
|
🚢 |
|
Btw, just noticed that recent patches aren't in |
Ocramius
changed the title
|
Also cherry-picking this into |
|
Interestingly, the test passes on |
…3.x`) suite and mapping changes
Providing a failing testcase as requested in issue #7062