FEATURE: Enable hierarchical asset collections

[Sebobo]

This allows assigning a parent to collections to enable the hierarchy feature in the Flowpack.Media.UI.

Upgrade instructions

Run ./flow doctrine:migrate.

Review instructions

The tests should cover the functionality.

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[Sebobo]

@bwaidelich as discussed the hierarchy change for AssetCollections targeted at 7.3.
Do you see anything missing or dangerous?

[bwaidelich]

I'm really happy about this approach and how little code it actually changes!

Just a couple of comments re circular dependencies and lazy loading

[Sebobo]

@bwaidelich thx for the check.

Honestly it's a bit confusing what parts the persistence layer takes care of and which I have to implement myself with those relations 😔
So I added more tests and will check if I need even more. Found 2 more bugs depending on how I modify the parent child relationship.

[bwaidelich]

When I check out this branch in a Neos 8.2 installation I get

PHP Fatal error:  Declaration of Neos\Media\Domain\Model\AssetCollection::setParent(?Neos\Media\Domain\Model\AssetCollection $parent): void must be compatible with Neos\Media\Domain\Model\AssetCollection_Original::setParent(?Neos\Media\Domain\Model\AssetCollection_Original $parent): void in Data/Temporary/Development/Cache/Code/Flow_Object_Classes/Neos_Media_Domain_Model_AssetCollection.php on line 513

o.O

[bwaidelich]

Related Media UI Change: Flowpack/media-ui#184

[kitsunet]

Wait why in 7.3 backwards? I am not totally against it, but introducing a feature (with a migration no less) is a bit sketchy?

[bwaidelich]

[...] introducing a feature (with a migration no less) is a bit sketchy?

FYI: It was declared a "TASK". But I just changed that, because that is surely a feature.

Something related I would like to discuss:
By making collections nestable, they act more like folders in the classic sense. IMO that should imply that an asset can only appear in one collection/folder at a time.
IIRC this constraint is checked in the Flowpack/media-ui#184 PR, but it might be something that we want to prevent on the lower levels (or make this configurable somehow).

I would suggest to target this for 8.3 or even 9.0. But we could try to extract an installable package with the required patches for Neos < 8 ?

[bwaidelich]

IMO that should imply that an asset can only appear in one collection/folder at a time.

Thinking about it again, that's probably too much of a change no matter what.. Probably we can solve this via UX (e.g. offer both ways when moving an asset to a collection) – I'm not sure what the current state of the UI change is since I can't get it to compile currently

[kitsunet]

Yes, that all sounds like a good suggestion, although migration might be a bit of a problem between a package and then later core, although if we name it the same version it could work?

Also checking that it is in only one collection at a time seems sensible. I don't want to start a huge dicsussion but wonder about some cases this opens that seem "complicated". And I don't understand how nested collections are supposed to work internally.

A collection has tags attached, how is that supposed to work with children? It seems counter intuitive that children do not inherit the tags?
A site (in Neos.Neos) has a collection attached, how is that supopsed to work with parents/children in the future then?

I probably missed a discussion about it but if those two were my choices, I still prefer making tags nestable (I know we reverted that) but collections really seems tricky?

[Sebobo]

We wanted to have nesting possibilities for assets in the Media UI with Neos 7.3 support which was discussed with the Media UI sponsors and involved people.
That's why I targeted Neos 7.3 after discussing it longer with @bwaidelich and deciding against a new entity type for folders.

Targeting Neos 9.0 would be a huge problem as most users will not be on a Neos 9 version for quite a while.
And 8.3 doesn't solve the 7.3 target mentioned above.

I have a AOP version of the folders in the Media UI folder branch currently, but this makes db migrations really hard. At least I don't know how I would prevent conflicts later.

In the future I would handle tags in the Media UI in a different way than the Media browser.
The media browser uses them to reduce the amount of selectable tags.
I think this makes little sense and rather use the tags of collections the same way as tags for assets. To tag them and make them findable later.

The connection between site and collection is unchanged and doesn't need a change IMO. We only need a new privilege for including child collections.

And regarding one asset / collection. I'm making this configurable in the Media.UI as I heard good reasons for both usages currently and don't want to force either way right now. But I would define the single collection for each as default and enforce it via the Media UI API

[bwaidelich]

Yes right, I remember, sorry..
IMO we could go for an exception in this case – especially if we find a way to make this work fully b/c – I didn't test what happens if the migration is not applied

[kitsunet]

I didn't test what happens if the migration is not applied

Doctrine unfortunately is nasty about that as it selects all defined properties of the objects it handles, so it will fail hard when the columns do not exist. I would still be kinda ok with doing this, I am pretty sure we had a couple of bugfix releaess with migrations before, but it definitely breaks if you don't migrate.

Still feels like conceptually this is a bit "wobbly" with the tags and site connection, but if there is awareness of those points and it seems to work, fine for now I guess.

[bwaidelich]

I think I missed some discussions about this, but I have the feeling that we should collect some more input in order to come up with a unified model that suits the most.

My personal preference would be one close to computer file systems because people will be familiar with it.

Thinking out loud:
Have folders and tags and a search that can be limited to either (e.g. search for "" with tags "" or "" in folder "foo", recursively).

And instead of connecting a site to an asset collection we could maybe use asset sources instead (i.e. more or less isolated DAMs).
With that there wouldn't be a requirement to limit tags to certain folders.
Also the notion of nested tags is really odd to me.

Disclaimer: This is just my personal feeling of the day and I don't really (have to) work with the media module on a daily basis. So it's perfectly possible that I'm missing some points. But I would love to discuss this once again and write down conclusions – and then derive next steps from it. maybe at the sprint?

btw: I wonder why we need both, setChildren() and setParent() – IMO it would simplify things if we just had the latter

[kitsunet]

I think I missed some discussions about this, but I have the feeling that we should collect some more input in order to come up with a unified model that suits the most.

My personal preference would be one close to computer file systems because people will be familiar with it.

Thinking out loud: Have folders and tags and a search that can be limited to either (e.g. search for "" with tags "" or "" in folder "foo", recursively).

Yes this would be my favorite is well

Also the notion of nested tags is really odd to me.

This was mostly a shortcut to above, allowing people the freedom to express either flat or neste structures. I don't care either way, and don't need tags to be nestable, and understand that concept might not be for everyone (and might result in a change of naming as well), but the idea of a hierarchical structure that allows multiple assignment can make sense (see gmail labels - which by concept were adopted by other mail providers). But either way I am totally fine to split the hierarchy from the tags, I just feel collection isn't it yet. (And it could be interesting to consider multi assigning to "folders" as well - maybe a switchable option?)

And instead of connecting a site to an asset collection we could maybe use asset sources instead (i.e. more or less isolated DAMs). With that there wouldn't be a requirement to limit tags to certain folders.

Then we need to change the concept of assigning one collection to a site, as you certianly might want multiple sources that can be assignet to all or just one or a few sites in your installation, and we need to make it easy to have multiple "neos" sources, which I think currently is not possible?

Disclaimer: This is just my personal feeling of the day and I don't really (have to) work with the media module on a daily basis. So it's perfectly possible that I'm missing some points. But I would love to discuss this once again and write down conclusions – and then derive next steps from it. maybe at the sprint?

Same, same 👍

[Sebobo]

I discussed the hierarchical thing, collections and tags with a bunch of people and currently the collections and tags are mostly used as a pseudo 2-level folder system. It's rare that they are used for anything else in my experience.
Some projects use privileges to limit access to some collections but this is also not perfectly implemented.

Though I would love to implement multiple Neos sources in the not so far future. As it would solve several use cases especially with a multi site installation, access or when trying to separate data from imports etc..

But back to the topic. How do we proceed here? :)
@bwaidelich there was a reason I added the setChildren but sadly I forgot. If I don't remember anymore I can also remove it.

[Sebobo]

@bwaidelich I removed the child operations but without the children property I cannot enforce the deletion of children when a parent is removed.

[bwaidelich]

I removed the child operations but without the children property I cannot enforce the deletion of children when a parent is removed.

We need a way to get all direct (or maybe even all descendant) collections anyways, but I would suggest to do that in the repository:

class AssetCollectionRepository {
    public function findChildCollections(string $collectionId) {
       // ...
    }

    public function findDescendantCollections(string $collectionId) {
       // ...
    }
}

They might require a little DQL magic to be fast, but that should work in general.
And with that in place, you could adjust AssetCollectionRepository::remove() to check to avoid orphaned collections

[Sebobo]

Hi @bwaidelich I adjusted the code to remove the children in the repo instead of letting doctrine do it via the annotation, the functional test is happy with the change.

[bwaidelich]

you never change that variable, can that work?

[Sebobo]

I actually think this is correct as we have a circle and the parent would always appear twice, except if there was a circle to begin with and we would attach to that circle.

But I will improve the code, error message and test labels

[Sebobo]

@bwaidelich I adjusted both migrations.

[bwaidelich]

Suggested change

	if ($parent === $this->parent) {
	if ($parent !== null && $parent === $this->parent) {

Without this null check I get a warning:

[bwaidelich]

+1 by reading (apart from the last nitpick) and a quick test

But as discussed we should consider targetting 8.4 (once that exists) with the option to backport this feature via composer-patches or AOP