getkirby / ideas

This is the backlog of ideas and feature requests from the last two years. Use our new feedback platform to post your new ideas or vote on existing ideas.
https://feedback.getkirby.com
20 stars 0 forks source link

Contains filter for pages, users and files fields #566

Open nilshoerrmann opened 4 years ago

nilshoerrmann commented 4 years ago

It's a common use case to create content relations with the pages, users or files fields. While this works pretty straightforward in the interface, on the front-end side filtering gets complex. Citing the docs:

Filter by pages field

For example, fetch all pages in a collection that reference the current page (or any other page) in the related field:

$page->children()->filter(function($child) use($page) {
    return $child->related()->toPages()->has($page);
});

This is very cumbersome when you have to do a lot of relation filtering. It would be very helpful if Kirby provided a has filter that would replicate above functionality:

$page->children()->filterBy('related', 'has', 'my-page-id');
$page->children()->filterBy('related', 'has', ['multiple', 'page', 'ids']);

There would also be the need for a reversed filter:

$page->children()->filterBy('related', 'has not', 'my-page-id');
$page->children()->filterBy('related', 'has not', ['multiple', 'page', 'ids']);

These filters could also have shortcuts using ? and !:

$page->children()->filterBy('related', '?', 'my-page-id'); // has
$page->children()->filterBy('related', '!', 'my-page-id'); // has not
nilshoerrmann commented 4 years ago

PS: This is not to be confused with the in and not in filters which look at things from a different perspective checking if the left value is in the right definition. This is about checking whether the right value is part of the left.

nilshoerrmann commented 4 years ago

Maybe ! should be !? but I'm not sure if those operators are somehow cast for something else already in PHP land.

lukasbestle commented 4 years ago

How would the filter know what is stored in the fields? It could be pages, files or a list of completely different elements.

So to be honest, I think this is too specific to be included in the core. It would be great as a plugin however.

nilshoerrmann commented 4 years ago

The filter doesn‘t have to know what is stored in the field from my understanding:

If the filter accepted string values only as in my example, it would just have to convert the field value with the yaml() method and then check if the filter value is in the resulting array.

If the filter also accepted Kirby objects – a page, file or user to filter by – the filter function can deduce the context from this object so the info is there in plain sight.

nilshoerrmann commented 4 years ago

So to be honest, I think this is too specific to be included in the core. It would be great as a plugin however.

Why is this too specific if there are three core fields missing a simple filtering process? Simple in the sense that the given examples in the docs are complex PHP when I look at it compared to the other filters.

lukasbestle commented 4 years ago

If the filter accepted string values only as in my example, it would just have to convert the field value with the yaml() method and then check if the filter value is in the resulting array.

In your example above you used ->toPages()->has(), which won't work unless the filter knows the field type. But you are right that a rather simple "is the string part of the list inside the field" comparison would work no matter if it's pages, files or whatever.

But that leaves us with another issue: There can be different separators for the fields (a YAML list, a comma-separated list or a list separated with a completely custom character). Also the method on the objects could actually return something else than a string (for example an array with strings). Ideally the has filter would support all of these variants, but that's hard to implement correctly.

Why is this too specific if there are three core fields missing a simple filtering process?

I was referring to the original example with ->toPages() that would be too specific for a general has filter. See my first reply in this post.

nilshoerrmann commented 4 years ago

But that leaves us with another issue: There can be different separators for the fields (a YAML list, a comma-separated list or a list separated with a completely custom character). Also the method on the objects could actually return something else than a string (for example an array with strings). Ideally the has filter would support all of these variants, but that's hard to implement correctly.

But there are filters for these cases already, aren‘t there? Like $pages->filterBy('tags', 'article', ',') where the third parameter defines the separator? Wouldn‘t it be possible to do something similar and expect YAML otherwise?

$page->children()->filterBy('related', 'has', 'my-page-id', ',');

Also the method on the objects could actually return something else than a string (for example an array with strings).

When would that happen? Isn‘t an array of strings what we‘d need to do an in_array comparison?

lukasbestle commented 4 years ago

Yes, I forgot about the $split argument. That would be doable.

nilshoerrmann commented 4 years ago

I just noticed something interesting: if I use - as the split character for a pages field, it works exactly as the the contains filter I'm requesting. Example:

$page->children()->filterBy('related', 'my-id', '-'); // returns all pages containing 'my-id' in the related field

It seems like Kirby is internally trimming the white space when splitting which makes it possible to parse the YAML of the pages, users and files fields. So actually the has filter already exists and just isn't documented.

nilshoerrmann commented 4 years ago

I'm not sure if this is just happening by accident, but if not the docs should be updated accordingly because this simplifies content filtering a lot.

nilshoerrmann commented 4 years ago

As an additional note: it seems like Kirby does always store content for the pages, users and files fields using the list syntax – one item per row, prefixed with a dash – no matter if the field allows for single or multiple items. So it's

Users:

- asd9ofus

for single items and

Users:

- asd9ofus
- dfjh898d
- 24kj4ksg

for multiple. So splitting by - should always work 🎉

lukasbestle commented 4 years ago

I'm not sure if this is just happening by accident

It is. Your solution is a clever hack, but not an "official" feature I'd add to the docs.

nilshoerrmann commented 4 years ago

I think I don't agree that it's a hack because it's not misusing a function in a way it wasn't intended to be used. Str::split is supposed to split a string by a specified character, trim the result and remove empty items in the resulting array. This is exactly what we need to handle all cases which are either a dash-separated or a comma-separated or custom-separated list. We are never using a full YAML document to store references in pages, users or files fields, it's only the list style array notation of YAML. So this solution doesn't require any additional parsers (which should be good for performance) and thus there is no need for complex conditions to decide in the filter which method to use to generate an array (which makes the code more readable).

So I actually like the simplicity of the approach conceptually.


On a more general note, when I started using Kirby, I found the concept of defining a split character on filters or using the pluck method very counter-intuitive and overly complicated. This has to do with the number, order and function of parameters used in the filter methods. Usually, I would expect the logic to always be field, compares, term. E. g.:

$collection->filterBy('category', '=', 'article');
$collection->filterBy('date', '>', 'today');

I'm fine with the shortcut to skip the comparison argument for the most common equals filter. The number of arguments is reduced from three to two so I understand this is some kind of shortcut. To filter lists, I expected a comparison filter as well. Something like this:

// finding pages that have news selected in their tags field
$collection->filterBy('tags', 'has', 'news');
$collection->filterBy('tags', 'lists', 'news');

A custom split character could be part of that comparison argument, too. Or it could be a fourth argument:

$collection->filterBy('tags', 'lists ,' 'news');
$collection->filterBy('tags', 'lists', 'news', ',');

This would keep the logical order field, compares, term which is very understandable in the context of the other filters. Or it would extend the logical order by an additional argument field, compares, term, by which would make it clear that this filter is doing more than other filters (three vs. four arguments).

Instead, for filtering lists, we have field, term, split which also has three arguments but works completely different than the other filters with three arguments. It's like a shortcut within a shortcut. And this is what I'd call a "clever hack" 😉 I'd always avoid that for clarity reasons. This cleverness made it very, very hard for me to understand and memorize filtering in Kirby from the beginning. It's a big flaw in my eyes and the reason why I have to read the filtering compendium in the docs every single time I have to set up a filter.

So – independent of the implementation – I think it would help filtering in Kirby a lot to introduce a has/lists/contains filter for clarity reasons. I'd remove the current split implementation from the docs (while keeping it working of course) and move to the new has/lists/contains filter in the docs. And it would a allow for direct relationship filtering.

nilshoerrmann commented 4 years ago

PS: My guess is that filtering is the way it is for historical reasons, too. So while it made sense at the time and while you are used to it please think about it again with the eyes of a newbie. Thanks :)

nilshoerrmann commented 4 years ago

I created a small test plugin and this seems to work fine for pages, users and files fields as well as for character-separated content:

Kirby::plugin('hananils/has-filter', [
    'collectionFilters' => [
        'has' => function ($collection, $field, $test, $split = false) {
            foreach ($collection->data as $key => $item) {
                $reference = $collection->getAttribute($item, $field);

                if ($split !== false) {
                    $values = Str::split($reference->value(), $split);
                } else {
                    $values = $reference->yaml();
                }

                if ($reference && in_array($test, $values)) {
                    continue;
                }

                unset($collection->$key);
            }

            return $collection;
        }
    ]
]);

Filtering syntax:

/**
 * Pages:
 *
 * - page-1
 * - page-2
 */
$collection->filterBy('pages', 'has', 'page-1');

/**
 * Tags: this, is, good, news
 */
$collection->filterBy('tags', 'has', 'news', ','));

It could be enhanced to also allow for (unnested) structure fields:

/**
 * Structure:
 *
 * - 
 *  key: value
 */
$collection->filterBy('structure', 'has', ['key', 'value']);

Would be great to see something alike adapted in the core.

nilshoerrmann commented 4 years ago

@lukasbestle @bastianallgeier Is this something that should be moved to Nolt (I don't know how to best do this without loosing too much context) or is this something that can be converted to a normal issue?

lukasbestle commented 4 years ago

I'd say we can either leave this here for now or you can create a Nolt idea with a summarized description and a link to this issue. We will archive this repo at some point but the existing issues should still be accessible by URL.