Skip to content

Add support for extraction of associated entities to their ID's #153

Closed
davidwindell opened this Issue Jan 22, 2013 · 40 comments

6 participants

@davidwindell

At present, if one wants to use the Hydrator to extract an entities values, any related entities are left un-hydrated. This means that Select boxes on forms will not work without a custom strategy being added which extracts the related entities identifier, for example;

use Zend\Stdlib\Hydrator\Strategy\DefaultStrategy;

class SelectStrategy extends DefaultStrategy
{
    public function extract($value)
    {
        if ($value instanceof AbstractEntity) {
            return $value->getId();
        }
        return $value;
    }
}

This issue is a placeholder for discussions on whether this could be automated in the hydrator by checking, at the time of extraction, for the identifier (via metadata) and returning this instead of the related entity itself.

@Ocramius
Doctrine member

@davidwindell sounds more like a bug in the current hydrator to me...

@bakura10
Doctrine member

This is expected ocramius. Hydrating does not extract or hydrate recursively (for zfrrest we will need recursion for instance).

The problem of select is well known but I don't like the idea of a strategy. It can't really be done in the select element itself ?

@Ocramius
Doctrine member

@bakura10 I think we should make hydration symmetric. Therefore what about using a "get the identifier only" as a default?

@davidwindell

The main reason I'm against doing it in the element is that there's no class metadata available, so you'd still have to have a form of strategy on every element if your ID field names are different.

@davidwindell

@Ocramius I like that idea as the default, you could then potentially offer recursive extraction as another option

@Ocramius
Doctrine member

I think we should have a basic test like following:

public function testSymmetrical()
{
   $this->assertEquals($obj1, $hydrator->hydrate($hydrator->extract($obj1), $obj2));
}

That's a sane default, no?

@bakura10
Doctrine member

You have metadata in element as you have an objec manager.

@davidwindell

I wonder if we could have some sort of attach a strategy to a field type, so you could have all relations using one strategy (which has access to the metadata). You could then ask for all fields of type 'DateTime' to be extracted as a timestamp, etc..

@Ocramius
Doctrine member

We're lacking an API in common to handle type conversions right now. That's kind-of a PITA for all the ORM/ODMs

@bakura10
Doctrine member

Starting from ZF 2.1, I think you can use a ClosureStrategy... But of course you'll need it to attach to all your fields (or you extend Hydrator so that scalar fields have this strategy by default).

The get identifier only sounds sane but the problem is that I'm not sure this will work in context of forms if it returns identifier, because the nested fieldset won't be populated.

@bakura10
Doctrine member

But I still don't know why we could not do it here : https://github.com/doctrine/DoctrineModule/blob/master/src/DoctrineModule/Form/Element/Proxy.php

We have object manager, we have everything we need aren't we ?

@Ocramius
Doctrine member

@bakura10: yes, but that's still forms. I would not include forms in the entire hydrator discussion tbh

@bakura10
Doctrine member

Yes, you should, because this is the context where this is needed.

@davidwindell

I need this outside of forms too, for our API.

@Ocramius
Doctrine member

@bakura10 I'd use it outside forms context too.

The fact is that with a form I'd always use some custom logic to build the form itself, and probably a custom hydrator too (because you know your customers are evil and want you to suffer).

Anyway, I think the form should silently follow hydrator logic, which on the other side is not depending on forms.

@davidwindell

For reference, at present when there are multiple objects (i.e. toMany relationship), one has to add a Strategy like so;

class CollectionSelectStrategy extends AllowRemoveByValue
{
    public function extract($value)
    {
        $return = array();
        foreach ($value as $entity) {
            $return[] = $entity->getId();
        }
        return $return;
    }
}
@bakura10
Doctrine member

You should not inherit from AllowRemoveByValue, does not make sense. You should just inherit from AbstractCollectionStrategy.

Furthermore, it's always sure that the primary key is called "id", and it can be a compound collection. At some point you should use getIdentifierValues.

I'll try to think it more about it. Tbh I'd like to use this hydrator for ZfrRest too, but currently it lacks a little flexibility.

@davidwindell

@bakura10 why not inherit from AllowRemoveByValue? People still need the hydrate functionality that is in there, it's the extract that has the problem as it just returns the collection, not the IDs.

Compound keys can be discussed later.

@bakura10
Doctrine member

Exactly. My mistake.

@acaciovilela

I'm suffering with the issue too.

Do you got some solution for this issue?

@bakura10
Doctrine member

Currently, the fix is easy @acaciovilela, just create a custom strategy :). We'd like to have this built-in but we need to figure out the best way to do it =).

@acaciovilela

@davidwindell, do you have an example of this strategy implementation?

@davidwindell

There's one at the top of this pull request.

@davidwindell

@Ocramius where do you see this living, in a default strategy or in the hydrator itself?

@Ocramius
Doctrine member

@davidwindell I am not sure about the implementation details, but the base hydrator should be consistent with this one.

As already said:

public function testSymmetrical()
{
   $this->assertEquals($obj1, $hydrator->hydrate($hydrator->extract($obj1), $obj2));
}

If extract converts all association to thier identifiers only I'm fine with that too. That's probably the most common use case I guess, and this symmetry will be respected anyway

/cc @bakura10

@davidwindell

@Ocramius ok, and can we ignore compound keys, or should they be returned as an array of keys? Should we allow disabling of this conversion of a association or can it be fixed functionality?

@Ocramius
Doctrine member

@davidwindwll actually, I must say that identifiers (in metadata) are also represented as a key => value array, such as

array('id' => 123)

This is the most consistent way to do it. On the other side it is annoying to handle in forms, but this is the "correct~ish" way

@davidwindell

Update: We're actually starting to find having the related entity data available useful in our app, I think this should be opt-in if built.

@chrvadala

Hi!

I have added

class SelectStrategy implements StrategyInterface
{   
    public function extract($value) {
        $value->getId();
    }        
}

to my doctrineObject hydrator (ManyToOne field), but now I have this exception
Strategies used for collections valued associations must inherit from Strategy\AbstractCollectionStrategy

This is because there is this line https://github.com/doctrine/DoctrineModule/blob/master/src/DoctrineModule/Stdlib/Hydrator/DoctrineObject.php#L119

What's the actual best practiche to have a select element that work correctly?

ps. testSymmetrical +1

@davidwindell

@ArtigianoDelWeb you need two strategies, one for collections and one for single relations. Attach something like the below to your collection selects;

<?php

use Doctrine\Common\Collections\Collection;
use DoctrineModule\Stdlib\Hydrator\Strategy\AllowRemoveByValue;

class MultiSelectStrategy extends AllowRemoveByValue
{
    public function extract($value)
    {
        if ($value instanceof Collection) {
            $return = array();
            foreach ($value as $entity) {
                $return[] = $entity->getId();
            }
            return $return;
        }
        return $value;
    }
}

Specifically, note the extends AllowRemoveByValue

@chrvadala

I think that the code that you have posted works only with collections if ($value instanceof Collection) {
how can I do for single relations?

/**
* @ORM\ManyToOne(targetEntity="Application\Model\City")  
 * @Form\Required(true)
 * @Form\Type("DoctrineORMModule\Form\Element\EntitySelect")
 * @Form\Options({"label":"City:", "target_class":"Application\Model\City"})
 */
protected $city;
@davidwindell

Strategies used for collections valued associations

This suggest that the field you're applying the strategy to is a collection valued association.

@chrvadala

Built in doctrine strategies are only for collections, but i see that you have written a StatusSelectStrategy at the top of this issue and I think that it works with single relation.
is this right?

@davidwindell

@ArtigianoDelWeb I've updated the example select strategy to show how I currently have it. You may need to double check your entity mappings if you still have problems.

@chrvadala

@davidwindell Only one more question..
Which kind of hydrator are you using?

DoctrineModule/Stdlib/Hydrator/DoctrineObject
or
Zend/Stdlib/Hydrator/ClassMethods

@davidwindell

@ArtigianoDelWeb I'm using DoctrineModule/Stdlib/Hydrator/DoctrineObject. It is recommended for use with all Entities.

@chrvadala

@davidwindell
ok... I don't understand why your code doesn't throw this exception https://github.com/doctrine/DoctrineModule/blob/master/src/DoctrineModule/Stdlib/Hydrator/DoctrineObject.php#L121 (SelectStrategy that you have provided isn't an instance of AbstractCollectionStrategy),
but I will study better how this code works.
thank you for all ;)

bye ;)

@davidwindell

I'm closing this issue for now as strategies provide a workable solution.

@alexmiddeleer

I still think this would be a valuable feature to add, since it's a little strange that associated entities returned during extraction are just empty. I will use the custom strategy for now; thanks for coming up with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.