FEATURE: Assets Eel helper for Neos.Media

[Sebobo]

This helper provides access to assets in Neos by either searching by Tag, AssetCollection, or
a searchTerm combined with optionally Tags and an AssetCollection.

Review instructions

Add some assets to collections and tag them, then use the helpers in Fusion like this:

assetsByTag = ${Neos.Media.Assets.findByTag('MyTag')}
assetsByCollection = ${Neos.Media.Assets.findByCollection('MyCollection')}
assetsBySearchTerm = ${Neos.Media.Assets.findBySearchTerm('alice')}

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]

I thought also about adding datasource to allow the selection of tags and collections, but that would require adding Neos as dependency to Neos.Media, which is Quatsch.

But it would help a lot with making it possible to select Tags and Collections as node property combined with this helper.
So an alternative would be to add them to Neos.Neos instead or to Flowpack.MediaUI 🤔

And currently the searchTerm method is not including the optional tags and collection in its name. But somehow findBySearchTermAndTagsAndCollections is also silly...

[kitsunet]

I would suggest creating separete methods for the respective ObjectType|string inputs, eg. findByLabel and findByLabelName or something, same for the other. Makes for a cleaner interface?

[mhsdesign]

I would suggest creating separete methods for the respective ObjectType|string inputs, eg. findByLabel and findByLabelName or something, same for the other. Makes for a cleaner interface?

i think we should follow the mentality of the new eel helpers for the ESCR ... idont know exactly how they look like, but they must have the same challenge of dtos vs primitives ...

[kitsunet]

I would suggest creating separete methods for the respective ObjectType|string inputs, eg. findByLabel and findByLabelName or something, same for the other. Makes for a cleaner interface?

i think we should follow the mentality of the new eel helpers for the ESCR ... idont know exactly how they look like, but they must have the same challenge of dtos vs primitives ...

I would definitely hard suggest it there too 😂

[Sebobo]

And do you guys think I should add any other methods from the AssetRepo?

[bwaidelich]

i think we should follow the mentality of the new eel helpers for the ESCR

There is not really a (new) mentality as far as I know. But I would suggest to see Eel helpers as adapters from the loosely typed fusion world into a strictly typed PHP world and therefore keep union types where it makes sense (i.e. where multiple variants mean the same thing).
e.g. I found it rather confusing to have findByLabel and findByLabelName on a fusion level.

But I think @kitsunet's comment was re findByTag and I agree that findByTag and findByTagLabel are two different things.

More concretely:

I think this is totally fine and makes it easier to use a helper:

function someHelper(SomeValueObject|string|null $foo): ?string {
    if ($foo === null) {
        return null;
    }
    if (is_string($foo)) {
       $foo = SomeValueObject::fromString($foo);
    }
    // ... process
}

While for the following I would suggest to split it up:

function someHelper(SomeEntity|string|null $foo): ?string {
    if ($foo === null) {
        return null;
    }
    if (is_string($foo)) {
       $foo = $this->someRepository->findByFoo($foo);
    }
    // ... process
}

[markusguenther]

As @bwaidelich and @kitsunet talked about it. Do we want to adjust the helper like the proposal?

[bwaidelich]

As @bwaidelich and @kitsunet talked about it.

We haven't really talked about it and I might have misinterpreted Christians point.

Do we want to adjust the helper like the proposal?

Actually my suggestion was to keep it separate methods.. But thinking about it again: Tags are a special entity (and IMO they should have never been an entity but a value object)

So maybe it makes sense to adjust findByTag() to allow entity or string (=label) like Christian suggested because you won't have an instance of the tag in many places.

And maybe the same for findByCollection.. I'm not sure

[bwaidelich]

For this one it makes a lot of sense, too, IMO:

Suggested change

	/**
	* @param Tag[] $tags
	* @return QueryResultInterface<AssetInterface> | null
	*/
	public function search(string $searchTerm, array $tags = [], AssetCollection $collection = null): ?QueryResultInterface
	/**
	* @param Tag[]|string[] $tags
	* @return QueryResultInterface<AssetInterface> | null
	*/
	public function search(?string $searchTerm, array $tags = [], AssetCollection|string $collection = null): ?QueryResultInterface

[Sebobo]

So I revert back to my first version that allowed both strings and objects for both methods?

[bwaidelich]

So I revert back to my first version that allowed both strings and objects for both methods?

Sorry, I didn't follow this PR from the beginning and didn't know that this was the first version?
The suggestions just reflect my personal opinion and I'm not even sure about it – feel free to ignore me ;)

[bwaidelich]

OK, I think I finally understood Christians comment (I had read it looking at the current version).

I'd say: Lets keep the separate methods as is.
But maybe we can change search as suggested? (i.e. allow term to be null and tags to be an array of strings)?

[mhsdesign]

I dindt completely follow the discussion, but can we later extract the conclusion into an issue or something, so we know how to write eel helpers in the future and dont have to re discuss this again ^^

[markusguenther]

The search does not work with tag labels, as the repository does not handle strings. If this is wanted, we need to transform all strings into tag instances first.

Also added some tests.

[markusguenther]

@bwaidelich Does this fit better?

[bwaidelich]

Does this fit better?

IMO it's great like this, but this is just my personal preference and @kitsunet opposed this solution if I got it right?

The search does not work with tag labels, as the repository does not handle strings.
If this is wanted, we need to transform all strings into tag instances first.

That's what I would expect to be honest, like:

$tagInstances = array_filter(array_map(fn (string|Tag $tag) => is_string($tag) ? $this->tagRepository->findOneByLabel($tag) : $tag));

[kitsunet]

It's fine by me, even though with the null (why did that come into play?) it now looks pretty convoluted with the type hints. I just think this will be hard to refactor later and fall on our feet sooner or later 😆 but if you all are fine with this, lets do it.

[bwaidelich]

with the null (why did that come into play?)

I'm not sure, but IMO it's a good idea to be fault tolerant in Eel helpers, i.e. don't throw an exception for ${some.helper(foo)} if foo happens to be null.
And many existing helpers already behave that way (either because they don't have type hints or because we explicitly allowed null, e.g. https://github.com/neos/neos-development-collection/blob/8.3/Neos.Fusion/Classes/Eel/BaseUriHelper.php#L31 or https://github.com/neos/neos-development-collection/blob/8.3/Neos.Neos/Classes/Fusion/Helper/NodeHelper.php#L56)

[Sebobo]

Yes. Often people feed variables into helpers which are not set yet, so we should be gentle.

[Sebobo]

@markusguenther I can make the tags adjustment in the search method later like we discussed. Thx for taking care of the PR while I was tripping.

[Sebobo]

I made the changes but now wonder, whether I shouldn't add a new search method to the AssetsRepository as search by tag OR collection is pretty useless IMO.

[markusguenther]

Wrote with @Sebobo yesterday, and we keep it as it is now.
So, the change of the AssetRepository can be a future improvement.