As written as one in the suggestions in #2044393: Changes in search query functionality:
Unify whether/where the query checks the validity of method parameters. I.e., we currently validate input for fields() and sort(), but not for condition(). (If we want to check there, too, the filter needs to be aware of the index, though – but that might make sense in any case.) Either we should check in all the methods, where it makes sense, or just once in execute() (would eliminate the possibility that other methods throw exceptions – probably a DX plus), or completely leave that to the service classes (bad UX because of code duplication).
However, I'd now argue that while moving the validation out of the query class entirely does lead to a bit of code duplication, but on the other hand it allows backends to easily change the definition of what constitutes "valid" values for all of the query data. This is important when features, e.g., introduce new fields (e.g., #1197538: Random sort in Views), new operators (something like #1783746: Add support for "between" operator (though we want to have that in the Search API itself now anyways)), etc.
Also, most of the values are currently double-checked, since the backend plugins tend to not trust the validation of the query class anyways.
Comment | File | Size | Author |
---|---|---|---|
#2 | 2604874-2--query_validation_move.patch | 14.44 KB | drunken monkey |
|
Comments
Comment #2
drunken monkeySomething like this.
(Note: This would also lead to changes in the Solr backend, and maybe other backends, becoming necessary.)
Comment #3
borisson_This change makes a lot of sense even though this'd break solr other backends, I think doing this now (before the official 8.0.0 release) is a good time.
Comment #4
drunken monkeyThanks for your input, good to hear others also think that this makes sense!
Committed.