At least 90% of the time when you call \Drupal\search_api\Query\QueryInterface::createConditionGroup(), you want to add that condition group to the query afterwards. (Sometimes, of course, you want to nest it inside another condition group instead.)
We should simplify this by adding a new third $add_directly parameter to createConditionGroup().
To maintain backwards compatibility, I guess for now we can’t just add that parameter to the interface (especially not defaulting to TRUE, which would make the most sense) – even though I really don’t think anyone actually provides a second implementation for it, let alone overrides the createConditionGroup() method.
So, the steps are probably as follows:
- Add the parameter just to
\Drupal\search_api\Query\Query::createConditionGroup()(i.e., in the class, not the interface), as?bool $add_directly = NULL. IfNULLis passed, trigger a deprecation notice that an explicit parameter needs to be passed, as the behavior will change in the future. For now, of course, this keeps defaulting toFALSE, the previous behavior.
Probably also document the parameter in the interface, but don’t actually add it in the code. Explain the situation in the doc comment. - In the next major release (2.0.0, probably), change the parameter definition to
bool $add_directly = TRUEand also add that parameter to the interface method.
To safeguard against any custom code that still does this wrong, we could also add a check toaddConditionGroup()to make sure the same group won’t be added twice. (Would not work for nested groups, though, of course, so still not 100% safe.)
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3260862-6--createAndAddConditionGroup.patch | 21.43 KB | drunken monkey |
Comments
Comment #2
drunken monkeyComment #3
drunken monkeyThe attached patch would implement this change. (As I noticed only halfway through that I went outside the module, there is also a patch that would update the Solr module accordingly.)
However, as this adds two additional parameters to most
createConditionGroup()calls, which is quite a large change, I’m not 100% convinced that we should do this. On the other hand, I expect most other modules will just have a handful of those calls at most.Anyways, input/feedback/opinions very much welcome!
Comment #4
mkalkbrennerWhat about introducing a second function named
createAndAddConditionGroup()? From my point of view that is more readable then parameters. Maybe as addition to the new parameters.Comment #5
drunken monkeyThat’s a great idea, thanks!
Seems pretty obvious now, but somehow I didn’t think of that. If we’d start from scratch, I think having
createConditionGroup()default to adding the condition would still make more sense, but now that there is already a lot of code out there using it, we’d probably indeed be better off just introducing a new method for that, instead of going through that weird deprecation dance to switch the method in time for 3.0.0 (or whatever faraway future version).The attached patch would implement this, please test/review! (No interdiff since that would be pretty much useless.)
One thing I’m not entirely sure about (cf. two hard things in computer science …) is the name of the new method, though.
createAndAddConditionGroup()does make sense, but I also likeaddNewConditionGroup()– basically replacing the second call (addConditionGroup()) instead of the first. What would you say?Any other opinions on this?
Comment #6
drunken monkeyAttaching a re-roll of #5.
In case no-one complains in the next week or so, I think I’ll just commit this version.
Comment #8
drunken monkeyCommitted and created a change record.
Comment #9
drunken monkey