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

Refresh Detached #6459

Closed
davidbarratt opened this issue May 21, 2017 · 14 comments
Closed

Refresh Detached #6459

davidbarratt opened this issue May 21, 2017 · 14 comments

Comments

@davidbarratt
Copy link

It would be really helpful if there was a way to refresh detached entities. This would essentially be the opposite of a merge.

Let's pretend you have a REST API that allows users to create Posts.

You might have a User entity and a Post entity and a relationship between the two. When a user submits a new Post, they should not have to also include all of the details of their user, they should only have to include the user id. The user entity should then be replaced with the one that is in the database (not the one submitted which lacks all of the fields).

The only options right now, are to use merge which would override the user's existing fields (with null for everything except the id), or to manually update the user entity on the Post after manually retrieving it from the database. This is fine for a simple example like this, but if posts had a lot of complex associations, this would get unwieldy quickly.

It would be really helpful if there was a method on entity manager to refresh detached entities (or maybe just change refresh to work this way)? Assuming there is an identifier for an entity, it would refresh it from the database, if the identifier is null or the entity is not found in the database, then it can be assumed that it is a new entity.

Having this would really help those pesky A new entity was found through the relationship errors, where you don't want to merge and you don't want to persist and you can't refresh (as in the example above).

@Ocramius
Copy link
Member

This can't be implemented, as it modifies an object graph that isn't under doctrine's control.

Doing so may lead to an EM modifying the state of an object under the control of another EM.

@Majkl578
Copy link
Contributor

I think this makes sense. Current merge behavior breaks object identity and makes it harder to work with. It'd be cool to have i.e. attach($object).
Ultimately it should be user's responsibility to not pass an object managed by another EM. EM itself shouldn't make such assumptions as it's not capable to do so (it doesn't know other EMs, or if they even exist).

@Ocramius
Copy link
Member

attach($e) cannot exist because the scope of the current entity manager may already be referencing an entity with the same identifier: that scope is not just​the EM, but all services that had contact with it.

@davidbarratt
Copy link
Author

doesn't that problem exist with the existing merge() method?

@Ocramius
Copy link
Member

Ocramius commented Jun 1, 2017

@davidbarratt merge() will retrieve a new merged instance, rather than attaching the actual object.

@davidbarratt
Copy link
Author

@Ocramius then I would expect attach() to work the same way. :)

@Ocramius
Copy link
Member

Ocramius commented Jun 1, 2017

FWIW, merge() and detach() are candidate for removal in 3.x anyway, soooooooo... ;-)

@davidbarratt
Copy link
Author

I mean if there is no attach() method (of some kind) someone would have to go through each associated property and load the entities. I don't mind writing this (since I have to eaither way), but it just seems really boilerplate.

@Ocramius
Copy link
Member

Ocramius commented Jun 1, 2017

I don't mind writing this (since I have to eaither way), but it just seems really boilerplate.

It's what tools like JMSSerializer do, which is why we are dropping this, as the complexity of the operation is extremely high.

You just fetch the root of what you need, then merge it with a serialized graph.

@davidbarratt
Copy link
Author

Which is what I do in my application, but that's a lot harder when the root does not exist. :( (hence this issue)

@davidbarratt
Copy link
Author

So I ended up creating a new service to do it:
https://github.com/geosocio/core/blob/develop/src/Utils/EntityAttacher.php
and I also had to create a new annotation because of the whitelist on cascade.

Let me know if you're interested in having this in doctrine and where it should go and I can create a PR. If not, feel free to close this issue. :)

@Ocramius
Copy link
Member

Ocramius commented Jun 4, 2017

That's kinda the point of the discussion above: it shouldn't be in core :-\

davidbarratt pushed a commit to geosocio/entity-utils that referenced this issue Jun 16, 2017
davidbarratt added a commit to geosocio/entity-utils that referenced this issue Jun 16, 2017
davidbarratt added a commit to geosocio/entity-attacher that referenced this issue Jun 23, 2017
@davidbarratt
Copy link
Author

I created a library for this if anyone is interested:
https://packagist.org/packages/geosocio/entity-attacher

@lcobucci
Copy link
Member

Closing it since @davidbarrat created a lib to handle that possibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants