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

fix: clear entity persister on restoring class metadata #264

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

wickedOne
Copy link
Contributor

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
Copy link
Owner

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

@wickedOne
Copy link
Contributor Author

wickedOne commented Mar 9, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

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

@wickedOne
Copy link
Contributor Author

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)

- added call to clearUnitOfWorkPersister when restoring class metadata
  in the flush method to be sure this library's id generator is no
  longer present in those
- added test to validate the entity manager doesn't throw an error
- fixed bug where bin files were not created for doctrine bridge
Copy link
Owner

@theofidry theofidry left a comment

Choose a reason for hiding this comment

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

Two tiny things to fix LGTM otherwise, thanks a lot for that

Makefile Outdated
@@ -221,6 +221,7 @@ vendor-bin/doctrine/composer.lock: vendor-bin/doctrine/composer.json
@echo vendor-bin/doctrine/composer.lock is not up to date.

vendor-bin/doctrine/vendor/phpunit: vendor-bin/doctrine/composer.lock
composer bin doctrine update $(COMPOSER_FLAGS) || true
Copy link
Owner

Choose a reason for hiding this comment

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

should be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@theofidry theofidry changed the title clear entity persister on restoring class metadata fix: clear entity persister on restoring class metadata Mar 17, 2024
- applied review suggestions
@theofidry
Copy link
Owner

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

@theofidry theofidry merged commit c05882a into theofidry:master Mar 18, 2024
28 of 61 checks passed
@wickedOne wickedOne deleted the entity-persister branch March 18, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants