Skip to content

Conversation

@ElGigi
Copy link
Contributor

@ElGigi ElGigi commented May 2, 2019

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

@froschdesign
Copy link

froschdesign commented May 2, 2019

@pmjones
Travis is really missing here! Do you have plans to add it? Is a PR welcome?

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

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.

Copy link
Contributor Author

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

@froschdesign froschdesign May 2, 2019

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 :)

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)

Copy link
Contributor Author

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?

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.

https://github.com/doctrine/orm/blob/master/docs/en/cookbook/entities-in-session.rst#entities-in-the-session

…and this is the path you should go.

Copy link
Contributor Author

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?

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 select method to get your domain object

…and the entire pull request is not needed.

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