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

Persist doctrine entities with DEFERRED_EXPLICIT change tracking policy #2226

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

paxal
Copy link
Contributor

@paxal paxal commented Oct 2, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? maybe
Deprecations? no
Tests pass? yes
Fixed tickets #2155
License MIT
Doc PR ~

As reported in #2155 doctrine entities are not persisted if change tracking policy is DEFERRED_EXPLICIT.

This implementation relies on ClassMetadataInfo, which is the ClassMetadata implementation. As it's API might change, I've added a test on method existence.

@soyuka soyuka merged commit 7a8674b into api-platform:2.3 Oct 3, 2018
@soyuka
Copy link
Member

soyuka commented Oct 3, 2018

Thanks @paxal !

@paxal paxal deleted the fix/doctrine_deferred_explicit_2.3 branch October 3, 2018 08:57
@paxal
Copy link
Contributor Author

paxal commented Oct 3, 2018

Thanks for your quick merge :)

@backbone87
Copy link

backbone87 commented Oct 4, 2018

why not just unconditionally call persist? there is no reason to do extra checks besides supports, doctrine will handle all special cases.

@soyuka
Copy link
Member

soyuka commented Oct 5, 2018

according to #2155 it does not?

@backbone87
Copy link

The problem is the contains call, which prevents the persist call for existing (but changed) entities. Just drop the whole condition around the persist and let doctrine do the checks (which It does anyway when persist is called)

@soyuka
Copy link
Member

soyuka commented Oct 5, 2018

The contains was added in #1782, reasons are stated there.

@backbone87
Copy link

I dont get the resoning there. What is meant with „Changes from the outside“ and how does a persist call override them. This sounds to me more like a missuse of doctrine. Persist calls should never override anything: https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/working-with-objects.html#working-with-objects

@soyuka
Copy link
Member

soyuka commented Oct 5, 2018

Mhh I think that it's in case of relations there were issues when persisting updated ones. Anyway, Doctrine doesn't check for contains and we definitely don't want to persist entities that aren't managed. From what I see in the UnitOfWork, persist doesn't do the check contains does. Also, the persist doc states:

NOTE: The persist operation always considers entities that are not yet known to this EntityManager as NEW. Do not pass detached entities to the persist operation.

@backbone87
Copy link

I think you missread the condition. Persist is called, when the uow does not contain the entity already

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.

4 participants