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

Add support for nesting embeddables #1105

Merged
merged 2 commits into from
Aug 21, 2014

Conversation

deeky666
Copy link
Member

This is a possible approach towards adding support for nesting embeddables (embeddables inside embeddables). I'm not sure whether this implementation is the best solution but I think it is something to start with. It seems to work flawlessly so far but I'm not sure if I missed something, so any feedback is welcome ;)

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3249

We use Jira to track the state of pull requests and the versions they got
included in.

foreach ($class->embeddedClasses as $property => $embeddableClass) {

if (isset($embeddableClass['inherited'])) {
continue;
}

if ($embeddableClass['class'] === $class->name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not the only case leading to an infinite nesting. You need to account for loops as well: A embed B embed A (and with any length for the cycle of course)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good way to solve this is to keep an internal reference map (new property in the CMF) somehow. Finding loops would be easy then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I almost expected to be missing something here. Meh that is tricky... Have to think about a reasonable solution here :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private $embeddablesInheritance = []; and then

private function checkEmbeddablesInheritance($embeddableName, $looping = [])
{
    // do the recursion stuff here
}

Something like that. I suggest starting from a test, as it makes it much easier to write the recursion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius thanks I will try that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another data structure would maybe even be cleaner: could just be a small, final InheritanceMap object/utility

@deeky666
Copy link
Member Author

@stof @Ocramius is this solution acceptable? Can you think of any other cases where an infinite loop could occur?

guilhermeblanco added a commit that referenced this pull request Aug 21, 2014
Add support for nesting embeddables
@guilhermeblanco guilhermeblanco merged commit 400acad into doctrine:master Aug 21, 2014
@deeky666
Copy link
Member Author

Awesome, thanks! :)

@auro1
Copy link

auro1 commented Oct 20, 2015

This seems to be broken again? If I have an entity, which has an embeddable, which has an embeddable the schema tool explodes with the message: Peasant::$priest.person.id does not exist

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.

6 participants