setAssetCollection() behaviour is strange/broken

[kdambekalns]

Description

The logic of asetAssetCollections() in Asset seems broken and is at least inefficient.

Steps to Reproduce

1. Go to

   neos-development-collection/Neos.Media/Classes/Domain/Model/Asset.php

   Lines 398 to 413 in 8d77820

   	public function setAssetCollections(Collection $assetCollections)
   	{
   	$this->lastModified = new \DateTime();
   	foreach ($this->assetCollections as $existingAssetCollection) {
   	$existingAssetCollection->removeAsset($this);
   	}
   	foreach ($assetCollections as $newAssetCollection) {
   	$newAssetCollection->addAsset($this);
   	}
   	foreach ($this->assetCollections as $assetCollection) {
   	if (!$assetCollections->contains($assetCollection)) {
   	$assetCollections->add($assetCollection);
   	}
   	}
   	$this->assetCollections = $assetCollections;
   	}

2. Mentally run the code
3. Be astonished

Expected behavior

• the asset is added to all passed and yet unassigned collections
• the asset is removed from any currently assigned collection that are not in the passed argument
• collections that are not changed, are not touched at all
• only collections passed to the method end up in $this->assetCollections

Actual behavior

• the asset is removed from all currently assigned collections, no questions asked
• the asset is added to all passed collections, no questions asked
• all currently assigned asset collections are added back to $this->assetCollections eventually, even though the may no longer have the asset assigned

Affected Versions

Neos: 2.3, 3.x and 4.0, all versions as of today (2018-06-28)

[kdambekalns]

A note to the assignees: I hope you can check this out and provide some insights on why this code is like that for years now, and why never someone found it strange. Maybe there is something I just don't grasp about this…

[kdambekalns]

Ok, playing around with this brought a new insight: Adding the "old" collections back to the Asset is need for them to be correcly removed. Might be due to owning side voodoo in Doctrine.

I still think unassigning from all existing collections can be skipped…

public function setAssetCollections(Collection $assetCollections)
{
    $this->lastModified = new \DateTime();
    foreach ($this->assetCollections as $existingAssetCollection) {
        if (!$assetCollections->contains($existingAssetCollection)) {
            $existingAssetCollection->removeAsset($this);
            $assetCollections->add($existingAssetCollection);
        }
    }

    foreach ($assetCollections as $newAssetCollection) {
        if (!$this->assetCollections->contains($newAssetCollection)) {
            $newAssetCollection->addAsset($this);
        }
    }

    $this->assetCollections = $assetCollections;
}

[grebaldi]

neos-development-collection/Neos.Media/Classes/Domain/Model/Asset.php

Lines 407 to 411 in 8d77820

	foreach ($this->assetCollections as $assetCollection) {
	if (!$assetCollections->contains($assetCollection)) {
	$assetCollections->add($assetCollection);
	}
	}

Imho these lines have no effect as long as the affected asset isn't being queried within the same request after the new collections have been set.

However, this is exactly what Flowpack.Media.Ui attempts to do. After changing the asset collections for an asset, said asset is immediately being returned by the GraphQL API. The behavior described above thus causes: Flowpack/media-ui#56

I fiddled around with the Asset model and was able to produce the correct behavior by simply removing lines 407-411. I have encountered no indication of unexpected Doctrine behavior. UPDATE: Well, apparently I called that too early. Doctrine does behave strangely without adding those collections back. I'll have a look into that.

My plan now is to prepare a fix targeting 7.3. I'm going to incorporate the code suggestion by @kdambekalns from above.

[kdambekalns]

Woah, thanks for digging out that really old issue! I had forgotten about it, TBH. ❤️‍🔥