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

Fix FieldDescription for multi-level embedded properties #830

Merged
merged 1 commit into from Jun 4, 2018
Merged

Fix FieldDescription for multi-level embedded properties #830

merged 1 commit into from Jun 4, 2018

Conversation

ghost
Copy link

@ghost ghost commented Apr 18, 2018

I am targeting this branch, because it's BC bug fix..

Closes #826

Changelog

### Fixed
- Fix FieldDescription for multi-level embedded properties

To do

  • Update the tests

@kunicmarko20
Copy link
Contributor

Can you add a test for this, please?

@ghost
Copy link
Author

ghost commented Apr 18, 2018

Tests updated

* @var bool
*/
protected $plainField;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please configure your IDE to always add a newline at end of file, this will make the red pictogram disappear.

Copy link
Author

Choose a reason for hiding this comment

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

Should be OK

@@ -249,6 +250,10 @@ public function testGetParentMetadataForProperty()
list($metadata, $lastPropertyName) = $modelManager
->getParentMetadataForProperty($containerEntityClass, 'associatedEntity.embeddedEntity.plainField');
$this->assertEquals($metadata->fieldMappings[$lastPropertyName]['type'], 'boolean');

list($metadata, $lastPropertyName) = $modelManager
->getParentMetadataForProperty($containerEntityClass, 'associatedEntity.embeddedEntity.subEmbeddedEntity.plainField');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break this one more time? We follow PSR-2 and your line shouldn't be longer than 120 chars

Copy link
Author

Choose a reason for hiding this comment

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

Done

@kunicmarko20 kunicmarko20 requested a review from a team April 18, 2018 13:06
@greg0ire
Copy link
Contributor

greg0ire commented Apr 28, 2018

From the docs:

Embeddables can only contain properties with basic @column mapping.

I'm not sure whether we should support this.

Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Please find an official source showing that multi-level embeddables are supported by Doctrine.

@bonfante
Copy link

bonfante commented Jun 2, 2018

@greg0ire
when looking to doctrine repo I can't find any doc supporting nested embeddables, but in there is a lot of PR to this behevour doctrine/orm/pull/1309, and it just works.
Just use only embeddables
I get similir issue with nested embeddables in the list. Would be greate to add this functionality.
But maybe is a good idea to change ModelManager::getMetadata too.
It was the first solution I found, but it still has issues with ['editable' => true] in configureListFields.

    /**
     * @param string $class
     *
     * @return ClassMetadata
     * @throws \RuntimeException
     * @throws \ReflectionException
     * @throws \Doctrine\Common\Persistence\Mapping\MappingException
     */
    public function getMetadata($class)
    {
        try {
            return $this->getEntityManager($class)->getMetadataFactory()->getMetadataFor($class);
        } catch (\RuntimeException $exception) {
            $metadata = $this->registry->getEntityManager()->getMetadataFactory()->getMetadataFor($class);
            if ($metadata->isEmbeddedClass) {
                return $metadata;
            }
            throw new \RuntimeException(sprintf('Failed to get metadata for class %s', $class),$exception);;
        }
    }

@greg0ire
Copy link
Contributor

greg0ire commented Jun 3, 2018

Looking at doctrine/orm#1311, it seems indeed to be somewhat supported

@OskarStark
Copy link
Member

Nice simplicifaction 👍

@OskarStark OskarStark merged commit de79063 into sonata-project:3.x Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not use multi-level embedded properties
4 participants