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:

  1. Add the parameter just to \Drupal\search_api\Query\Query::createConditionGroup() (i.e., in the class, not the interface), as ?bool $add_directly = NULL. If NULL is 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 to FALSE, 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.
  2. In the next major release (2.0.0, probably), change the parameter definition to bool $add_directly = TRUE and 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 to addConditionGroup() 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.)

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

Issue tags: +Needs change record
drunken monkey’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new11 KB
new25.3 KB

The 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!

mkalkbrenner’s picture

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.

What 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.

drunken monkey’s picture

StatusFileSize
new22.52 KB

That’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 like addNewConditionGroup() – basically replacing the second call (addConditionGroup()) instead of the first. What would you say?

Any other opinions on this?

drunken monkey’s picture

Attaching a re-roll of #5.
In case no-one complains in the next week or so, I think I’ll just commit this version.

  • drunken monkey committed d80778b5 on 8.x-1.x
    Issue #3260862 by drunken monkey, mkalkbrenner: Added new method...
drunken monkey’s picture

Status: Needs review » Fixed

Committed and created a change record.

drunken monkey’s picture

Issue tags: -Needs change record

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.