Automate collection merging

[bakura10]

Following my last changes : I added a Collection utils function that allow the user to automatically "intersect/union" elements from an old collection and a new one, so that the new elements of the new collection are added to the old one, and the ones removed from the new collection to be removed from the new collection too.

It "felt" bad to write the CollectionUtils::intersectionUnion in every entity method, so now it's completely automated and done in the hydrator (the test was quite a nightmare to write tbh).

So this :

public function setSkills(ArrayCollection $skills)
    {
        foreach ($skills as $skill) {
            $skill->setStudent($this);
        }

        $this->skills = CollectionUtils::intersectUnion($this->skills, $skills);

        return $this;
    }

now become :

public function setSkills(Collection $skills)
    {
        $this->skills = $skills;

        foreach ($skills as $skill) {
            $skill->setStudent($this);
        }

        return $this;
    }

But as you can see there is one change : every method type-hinted has to be changed from ArrayCollection to Collection. The reason is because as the merge is done directly in the hydrator, it returns not an ArrayCollection but a Doctrine\ORM\PersistentCollection, so we have to typehint with Collection instead.

It is therefore a little BC, but as this CollectionUtils is really new, I think that it's worth it.

I'll change the doc to reflect those changes as soon as it is merged.

[Ocramius]

As discussed on IRC, people should use Collection and ArrayCollection anyway, so the BC is negligible.

[Ocramius]

Suggested example is bad practice since the collection should be immutable. Will think about it.

[bakura10]

Well, if you think something is bad about Collections... just say me, we'll refactor it.

[Ocramius]

@bakura10 I think it is ok for now. We just need to go through all our collections stuff and check it briefly (security audit). Failing test cases should also not be a problem to write...

[davidwindell]

@bakura10 what if we don't want to use your collection merging logic, how do we implement this or are you forcing this on all users? This would be a problem for us as we only ever add to existing Collections but don't remove from them via forms.

[Ocramius]

@davidwindell expose immutable collections:

public function getMyCollection()
{
    return new ArrayCollection($this->myCollection->toArray());
}

Merging

[davidwindell]

@Ocramius can you clarify, I don't want my ArrayCollections changed to arrays just because the modules hydrator is forcing some merging logic on us? How does changing the getter affect hydration- I see no checks for ArrayCollection in hydrate()?

[Ocramius]

@davidwindell yeah, sorry :) I first merged since I long discussed this with @bakura10 in the last days.

Basically, this is an issue of how you (you intended as generic user) design your entities. By exposing your collection by reference, your object graph becomes really vulnerable to changes. If you want to protect that behavior, you have to make it immutable, either how I've shown in my example or by wrapping into an object that protects it.

The functionality introduced here has the advantage that the collection is not swapped within the entity (very common mistake, makes it easy to have really broken things) and merging is applied without removing elements (which breaks orphanRemoval).

It basically works on both defensive and non-defensive approaches in pretty much the same way.

[davidwindell]

@Ocramius Thanks for the breakdown and chat on IRC, however having now implemented this, I've had to comment out the check in my setters for whether the existing collection contains the new entity being added, i.e.

    /**
     * Set something
     *
     * @param ArrayCollection|Something[] $somethings
     * @return Other
     */
    public function setSomething(ArrayCollection $somethings) {
        foreach ($somethings $something) {
//            if ($this->somethings->contains($something)) {
//                return;
//            }
            $this->somethings->add($something);
            $something->setOther($this);
        }
        return $this;
    }

Otherwise it just returns everytime and never lets the setOther() call get made on the entities which are somehow now in $this->somethings?

[Ocramius]

@davidwindell that should be a continue, not a return :)

[davidwindell]

@Ocramius thanks, have changed that to continue, but its still not working, I get a DB exception because the setOther call never gets made.

I don't get how the $this->somethings is populated with data on a new entity (since this PR was merged) which is the cause of the problem I think

[davidwindell]

@Ocramius drilling down, there's something in the new merge code that populates the entities $somethings variable before calling the setter, I think..By removing the new code it works again

[bakura10]

Hi david,

The collection you receive in your setter ($somethings in your case) has already been merged. This means that $this->somethings === $somethings. They are the same object. So you are doing it wrong.

HOWEVER, as new items may have been added in the collection, you still may need to set the relation for inverse sides. So this is :

public function setSomething(ArrayCollection $somethings) {
        $this->somethings = $somethings;

        foreach ($this->somethings $something) {
            $something->setOther($this);
        }
        return $this;
    }

ocramius suggested that you did the intersectionUnion also in the setter (if you always use the Hydrator, no problem, but if another dev has the bad idea to call the setter without doing it correctly, boom). So it becomes :

public function setSomething(ArrayCollection $somethings) {
        $this->somethings = CollectionUtils::intersectionUnion($this->somethings, $somethings);

        foreach ($somethings $something) {
            $something->setOther($this);
        }
        return $this;
    }

If you are using the hydrator, as $somethings === $this->somethings, it will do nothing in intersectionUnino, but at least you have a secured code.

[bakura10]

"I don't get how the $this->somethings is populated with data on a new entity (since this PR was merged) which is the cause of the problem I think"

I used the classmetadata factory to retrieve the reflection class, so that I could get the value in hydrator.

[davidwindell]

@bakura10 Thanks for the breakdown, that's understood now thanks. But I still only want to only allow 'adding' and not 'deleting' via the hydrator so how can I prevent the deletions?

So for example, someone wants to add a new category to an existing product (via an API), if they only send the new category ID in the categories field, the old categories will all be removed as it stands.

[Ocramius]

@davidwindell use the code you pasted above together with a getSomethings that returns an immutable collection

[davidwindell]

@Ocramius I now have this, is this correct? Will it only add during merge/hydrate and never delete?

    /**
     * Get attachments
     *
     * @return ArrayCollection
     */
    public function getAttachments() {
        return new ArrayCollection($this->attachments->toArray());
    }

    /**
     * Set attachments
     *
     * @param ArrayCollection|Attachment[] $attachments
     * @return TicketPost
     */
    public function setAttachments(ArrayCollection $attachments) {
        foreach ($attachments as $attachment) {
            $attachment->setPost($this);
        }
        return $this;
    }

[Ocramius]

    /**
     * Get attachments
     *
     * @return ArrayCollection
     */
    public function getAttachments() {
        return new ArrayCollection($this->attachments->toArray());
    }

    /**
     * Set attachments
     *
     * @param ArrayCollection|Attachment[] $attachments
     * @return TicketPost
     */
    public function setAttachments(ArrayCollection $attachments) {
        foreach ($attachments as $attachment) {
            if ($this->attachments->contains($attachment)) {
                continue;
            }

            $this->attachments->add($attachment);
            $attachment->setPost($this);
        }

        return $this;
    }

[bakura10]

Moreover, if you are using Collection element in Zend, remember to set the flag allow_remove to false.

I really miss this from C++ :

public const Collection myFunction()...

[bakura10]

By the way david please re-read again my preivous message with examples, I corrected some errors

[davidwindell]

@bakura10 I don't think your example will work for me, because it will remove anything that isn't posted from the collection? @Ocramius I will try your example now.

[davidwindell]

Right, a combination of your two examples leads me to this conclusion, does it look correct?

    /**
     * Set attachments
     *
     * @param Collection|Attachment[] $attachments
     * @return TicketPost
     */
    public function setAttachments(Collection $attachments) {
        foreach ($attachments as $attachment) {
            if ($this->attachments->contains($attachment)) {
                $attachment->setPost($this);  <<< NOTE THIS LINE
                continue;
            }
            $this->attachments->add($attachment);
            $attachment->setPost($this);
        }
        return $this;
    }

I'm setting the inverse side even if already in the collection as pointed out above.

[bakura10]

Yes, you're right @davidwindell my example would remove anything that isn't posted.

Your code looks right =).

[davidwindell]

YAY! Thank you both, and a final question, about type hinting now, should we now use;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;

And then Type hint the setters on 'Collection' and everything else, i.e. the construct use 'ArrayCollection':

public function __construct() {
    $this->attachments = new ArrayCollection();
}

[Ocramius]

Yes, type hinting should always happen via Collection. Doctrine itself doesn't always use ArrayCollection :)

[davidwindell]

Thanks @Ocramius, I'm having another problem now :(, since the above, when I do a getAttachments() during the same request on the updated entity, I only get the newly added attachment.

When I do a second request, both original and new attachments appear. Can you advise?

[Ocramius]

What's the current status of your entity?
Marco Pivetta

http://twitter.com/Ocramius

http://marco-pivetta.com

On 10 October 2012 14:41, David Windell notifications@github.com wrote:

Thanks @Ocramius https://github.com/Ocramius, I'm having another
problem now :(, since the above, when I do a getAttachments() during the
same request on the updated entity, I only get the newly added attachment.

When I do a second request, both original and new attachments appear. Can
you advise?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/96#issuecomment-9300268.

[davidwindell]

@Ocramius it's STATE_MANAGED;

I think it might be the IntersectUnion modifying the original value of the entity by reference at $previousValue = $propertyRefl->getValue($object); causing it to not be updated?

[davidwindell]

@Ocramius yes, that's it, the line;

foreach ($toRemove as $key) {
    $collection1->remove($key);
}

is causing the problem because it's modifying the original objects value

[bakura10]

Yeah... This behaviour seemed logical to me to remove elements from the original collection if it's not present in the new one. What kind of use case don't you need that ?

But if this is problematic we should create another helper util (what you want is a union I suppose).

I think you could add a "union", "intersection", in addition to the "intersectionUnion". But then, we need to find a flexible way for the hydrator to use the right strategy. Don't have idea though.

[davidwindell]

@bakura10 this is for an update, so say we get an input of attachments[38] to an entity that already has attachments[29,34], the whole point of what i've been trying to work out in this discussion is to just add, not delete. It now works with my last snippet, BUT it deletes it from the inmemory object, argh

[bakura10]

Ha. And I suppose that if you add "orphanRemoval=true" in your DoctrineMapping, it will be removed from database too. This is exactly what I wanted to achieve in my use case. What about add hidden input for existing elements ? So that they will still be post and hence retrieved. This may do the trick !

[bakura10]

So basically

<input type="hidden" name=attachments[][id] value=29; ?>
// Same for 34
// normal field for your new one

[bakura10]

Anyway, I may have an idea for a fix. I'll give it a try tonight and you'll give me your opinion on it.

[davidwindell]

Thanks @bakura10, the forms are used via our API so there is no concept of hidden fields available.

[davidwindell]

Maybe we could check for a 'setter' in the entity, if there isn't one, use an 'adder'? So in my case, I would only have an 'addAttachments' function

[bakura10]

I don't really like this idea, I don't want to force people to create setter or adder, and do some behaviours based on the existence (or not) of such setters/adders/getters.

I'll try something else tonight :). I think it'll do the job ;-).

[Ocramius]

@bakura10 Symfony uses addSomething and removeSomething (which is not
bad imo). It is quite complex to handle though.

Marco Pivetta

http://twitter.com/Ocramius

http://marco-pivetta.com

On 10 October 2012 16:09, Michaël Gallego notifications@github.com wrote:

I don't really like this idea, I don't want to force people to create
setter or adder, and do some behaviours based on the existence (or not) of
such setters/adders/getters.

I'll try something else tonight :). I think it'll do the job ;-).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/96#issuecomment-9304153.

[bakura10]

I'm ok and I can do that but if I understand it well this means you add a check in intersectionUnion : if a removeSomething is defined THEN remove element, otherwise don't remove ?

What if the user define a remove but don't want them to be removed as in David use case ? I find it quite fragile as it would implies that the behavior of the hydrator is completely different if you add or remove a function.

My idea is to set a strategy in the hydrator (intersectionUnion as the default, but also union and intersection). So that the hydrator uses the good strategy. The problem is that it would be set globally to the hydrator. But we may introduce some kind of wild cards things... Going to the cinema and I give it a try as I come back guys ;-).

Envoyé de mon iPhone

Le 10 oct. 2012 à 16:14, Marco Pivetta notifications@github.com a écrit :

@bakura10 Symfony uses addSomething and removeSomething (which is not
bad imo). It is quite complex to handle though.

Marco Pivetta

http://twitter.com/Ocramius

http://marco-pivetta.com

On 10 October 2012 16:09, Michaël Gallego notifications@github.com wrote:

I don't really like this idea, I don't want to force people to create
setter or adder, and do some behaviours based on the existence (or not) of
such setters/adders/getters.

I'll try something else tonight :). I think it'll do the job ;-).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/96#issuecomment-9304153.

—
Reply to this email directly or view it on GitHub.