theofidry / AliceDataFixtures

Nelmio Alice extension to persist the loaded fixtures.
MIT License
311 stars 71 forks source link

fix: clear entity persister on restoring class metadata #264

Closed wickedOne closed 7 months ago

wickedOne commented 9 months ago

restoring the class metadata in the metadata factory does not restore the one doctrine has cached in the class property of it's persisters....

downstream this might lead to unexpected behaviour: when the id generator has been replaced with this library's IdGenerator, the basic entity persister does not recognise it as an id generator and treats the identity field as one which can be used for inserts in the BasicEntityPersister::getInsertColumnList method

this can lead to Invalid parameter number: number of bound variables does not match number of tokens errors on subsequent inserts of the entity manager. this change will fix that

theofidry commented 8 months ago

Thanks for the PR @wickedOne, sorry for the late review. Would there be a way to capture this behaviour in the tests?

wickedOne commented 8 months ago

no probs: better late than never :-)

as the persister's class property is not publicly available, it would require some hacky trickery. if that's ok with you, i can probably come up with something

theofidry commented 8 months ago

does it require to touch the persister class though (the test)? From your PR description, I thought this was somewhat reproducible by loading some fixtures and then doing inserts

wickedOne commented 8 months ago

ah ofc yes, the other way around :-) yes, that should be possible, i'll giv that a go next week

wickedOne commented 7 months ago

added test which makes sure the entity manager doesn't crash after flushing the entity persister. so basically remove my modification to the entity persister to see that test fail

the modification to the makefile is somewhat unrelated, but was necessary for me to run it locally (bin directory wasn't created in the vendor bin for doctrine)

theofidry commented 7 months ago

I checked the failures and they look unrelated, thank you @wickedOne