Add example for combining #293
Conversation
| 'collectionClass' => PostsFilterCollection::class, | ||
| ]); | ||
| ``` | ||
| This would add additional filters on top of inherited `status` one. |
There was a problem hiding this comment.
Technically the ones you add in searchManager() are the additional ones that are added to the ones in PostsFilterCollection.
There was a problem hiding this comment.
Interesting, as it still overwrote the ones in the filter for me.
I will have to test that once more.
Maybe it appends it and PHP just uses the last config in array found by key ;)
There was a problem hiding this comment.
Yeah using the same name will override, so if PostsFilterCollection already had a filter with name status it will be overwritten through searchManager().
There was a problem hiding this comment.
Which in the end means what I wrote though ;) Even if the technical detail might be a bit different.
There was a problem hiding this comment.
I wonder if we should fix the code here to first use the generic ones, then add the custom ones (overwriting).
If you configure a custom collection one would expect those to take precedence IMO.
Rare case, sure. Thus we can probably easily fix this in a minor release.
There was a problem hiding this comment.
There's no merging of filters/collections done anywhere. The use of collectionClass decides which collection class is instantiated and used for the name "default". By default search manager creates a FilterCollection instance which doesn't have have filters preset if no collectionClass config is specified.
There was a problem hiding this comment.
Feel free to rephrase this PR if needed, then we can probably merge and close.
I guess I don't want to deep dive too much into this - even if there was some value for me.
I just then go either full class or full table for mine for now.
There was a problem hiding this comment.
I just then go either full class or full table for mine for now.
That's the best approach. I personally would never specify collectionClass and use searchManager() too to modify the collection instance. That's why I don't want to document this and put ideas into people's minds 🙂.
Just the use of collectionClass is already documented.
There was a problem hiding this comment.
I think it is important to mention the side effects of them used together
In my upgraded apps I have tons of Table ones still
And in some cases I now added custom ones on top, and the intuitive expectation is for those to take full precedence or overwrite. While keeping the behavior for the other actions (admin prefix and otherwise) that have no such custom ones added at runtime yet.
now when you search in those you have weird effects that you cannot explain (and are silent, since it doesn't throw errors or warnings).
There was a problem hiding this comment.
I think it is important to mention the side effects of them used together
Sounds fair, I'll think of a suitable note.
|
Did you have a change to follow up on this? also note the header type change that is part of it. |
also fixed header
I found this out when trying to set a custom one but it still used the "inherited" one.