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

Dont autopersist in TranslatableEntityHandler #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DjLeChuck
Copy link
Member

I think TranslatableEntityHandler should not call persist on the newly created entity, this job should be done by the developer.

When using the TranslatableCRUDController for Sonata this modification will be transparent because a persist and a flush are already done.

Furthermore, in tests, the persist was done almost every time so the remaining are fixed and it's now consistent.

I'm open to any suggestion, pros or cons!

@artggd
Copy link
Contributor

artggd commented May 18, 2019

This looks good to me but we have got to test it out on existing projects and make sure this doesn't break BC. Otherwise we'll have to bump version to v2. If so, then let's have a talk on what could be improved on the next version 😉

I'll try and free some of my time next week to proof this PR against our recents projects.

@artggd
Copy link
Contributor

artggd commented May 18, 2019

And thanks for your work BTW @DjLeChuck!

@artggd
Copy link
Contributor

artggd commented May 18, 2019

The readme would also need to be updated to comply /w this PR 🙂
The part explaining the entity is translated and the examples too (cascade=persist).

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