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

Support embedded object for mapping #578

Merged

Conversation

claudusd
Copy link
Contributor

@claudusd claudusd commented Jun 1, 2016

Changelog

### Fixed
- Support embedded object for mapping

Subject

This PR allow to use embedded object in the show mapper.

class Foo {
    public function getValue()
    {
        return 'myValue';
    }
}

class Bar {
    public function getFoo()
    {
        return $this->foo;
    }
}
protected function configureShowFields(ShowMapper $showMapper)
{
    $showMapper
        ->add('foo.value');
}

The FieldDescription method getValue ignore the embedded object and do

$bar = new Bar();
$bar->getValue()

With my PR the getter call is on the embedded object and not on the parent object.

$bar->getFoo()->getValue()

@claudusd claudusd force-pushed the fix-embedded_on_show_mapper branch from 5c99a46 to 2978eb6 Compare June 1, 2016 18:54
@soullivaneuh soullivaneuh self-assigned this Jun 2, 2016
@soullivaneuh
Copy link
Member

I can't evaluate the real benefit because I tried this on 3.x:

// BankAccountAdmin
protected function configureShowFields(ShowMapper $showMapper)
{
    $showMapper
        ->add('customer.email')
    ;
}

Where customer is a User instance and it already works:

image

@greg0ire
Copy link
Contributor

greg0ire commented Jun 2, 2016

@claudusd : are you referring to this ? It's unclear.

@OskarStark
Copy link
Member

I think he mean if class Foo has no Admin class, does this work in your case @soullivaneuh ?

@claudusd
Copy link
Contributor Author

claudusd commented Jun 2, 2016

@greg0ire Yes i referring to the doctrine Embeddables.

@greg0ire
Copy link
Contributor

greg0ire commented Jun 2, 2016

@greg0ire Yes i referring to the doctrine Embeddables.

Then I am 👍 for your PR! We don't have any support for Embeddables yet, so you might run into other problems later, but we would love PRs to make it work.

@greg0ire
Copy link
Contributor

greg0ire commented Jun 2, 2016

LGTM

@soullivaneuh
Copy link
Member

I would like to finish my test before merge.

@greg0ire
Copy link
Contributor

greg0ire commented Jun 2, 2016

Also : it would be nice to document the support for this feature, once we are sure it works in most situations (so maybe in a later PR).

@@ -98,6 +98,10 @@ public function getValue($object)
$object = $this->getFieldValue($object, $parentAssociationMapping['fieldName']);
}

if (isset($this->getFieldMapping()['declaredField'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment about why this condition is here?

Something simple like this:

// Support embedded object for mapping.
// @see http://doctrine-orm.readthedocs.io/projects/doctrine-orm/en/latest/tutorials/embeddables.html

@soullivaneuh soullivaneuh changed the title Support embedded object for the show mapper Support embedded object for mapping Jun 2, 2016
@soullivaneuh
Copy link
Member

@claudusd Your PR does not fix only show mapper but also list.

Could you please change the commit title to:

Support embedded object for mapping

Thanks.

@claudusd
Copy link
Contributor Author

claudusd commented Jun 2, 2016

Ok, i will change the commit message and add documentation.

@soullivaneuh
Copy link
Member

Good! BTW I tested your PR manually, works great, good job! 👍

@claudusd claudusd changed the title Support embedded object for mapping [WIP] Support embedded object for mapping Jun 2, 2016
@claudusd
Copy link
Contributor Author

claudusd commented Jun 2, 2016

Don't merge, I will add more documentation this evening.

@@ -98,6 +98,12 @@ public function getValue($object)
$object = $this->getFieldValue($object, $parentAssociationMapping['fieldName']);
}

// Support embedded object for mapping
// Ref: http://doctrine-orm.readthedocs.io/projects/doctrine-orm/en/latest/tutorials/embeddables.html
if (isset($this->getFieldMapping()['declaredField'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, beware this syntax (Function array dereferencing) is only supported from php 5.4 (http://php.net/manual/en/migration54.new-features.php)

Copy link
Contributor

Choose a reason for hiding this comment

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

We have been having a problem with Travis, that's why the build wasn't red, but the next commit will probably report this.

@claudusd claudusd force-pushed the fix-embedded_on_show_mapper branch from 60f5167 to 043e042 Compare June 3, 2016 07:26
@claudusd claudusd force-pushed the fix-embedded_on_show_mapper branch from 043e042 to 9189b5d Compare June 3, 2016 07:30
@claudusd claudusd changed the title [WIP] Support embedded object for mapping Support embedded object for mapping Jun 3, 2016
@claudusd
Copy link
Contributor Author

claudusd commented Jun 3, 2016

I fix php 5.3 issue, rebase and add doc.

@soullivaneuh soullivaneuh merged commit 3ba0495 into sonata-project:3.x Jun 3, 2016
@soullivaneuh
Copy link
Member

Thank you @claudusd! 👍

@soullivaneuh
Copy link
Member

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.

None yet

6 participants