-
Notifications
You must be signed in to change notification settings - Fork 3
New Transit::attach() method to attach domain to the current context #16
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
base: 0.x
Are you sure you want to change the base?
Conversation
|
@pmjones Related to my question in atlasphp/Atlas.Orm#98 (comment) |
src/Transit.php
Outdated
| /** | ||
| * Attach a domain object to plan. | ||
| * | ||
| * @param \Atlas\Transit\object $domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class doesn't exists \Atlas\Transit\object. Use object instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed ;)
Due to #14
| * @return \Atlas\Transit\Transit | ||
| * @throws \Atlas\Transit\Exception | ||
| */ | ||
| public function attach(object $domain): Transit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a problem here: which method should the end user use? store or attach? This results in an unclear API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like Doctrine; it's needed to "attach" ("merge" with Doctrine) the entity to Transit before modification.
If the entity is modified and stored, she is already present and an update is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have an another suggestion of method name, i'm ready :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doctrine no longer works as an example, because merge and detach are deprecated and will be removed in version 3. 😉
https://github.com/doctrine/orm/blob/master/UPGRADE.md#bc-break-removed-entitymanagermerge-and-entitymanagerdetach-methods
doctrine/orm#1577 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i see that 👍 .
But how do for the current problematic?
It's the job of a framework? or need to integrate a concept into Transit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also at Doctrine:
It is a good idea to avoid storing entities in serialized formats such as
$_SESSION: instead, store the entity identifiers or raw data.
…and this is the path you should go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not too similar to Transit::select(string $domainClass, array $whereEquals = []) method; with second parameter who accept a scalar or array argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not too similar to…
You are on the right track. 😉
Don't serialize your domain objects, instead:
- store the entity identifiers or raw data (in session, etc.)
- use the existing
selectmethod to get your domain object
…and the entire pull request is not needed.
New Transit::attach() method who permit to add a domain to the handler storage.
It's permit to update a domain from another context, without fetch domain again before ; like domain getted from session.
Similar with Doctrine: https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/cookbook/entities-in-session.html
Issue: #15