Declare that ConditionGroupInterface or ConditionGroup implements \Stringable

I've created two merge requests...

1. !117, with one of the proposed solutions (i.e.: Declare that ConditionGroupInterface implements Stringable); and
2. !118, with the other proposed solution (i.e.: Declare that ConditionGroup implements Stringable)

When we decide on which of the two solutions is better, let's close the other merge request without merging.

Feedback welcome.

drunken monkey → closed merge request !117

drunken monkey → made their first commit to this issue’s fork.

Thanks a lot for proposing this change!
You’re right, of course, that we should properly advertise that condition groups are stringable.

Since ConditionGroupInterfacehas a 1:1 relationship with a class (ConditionGroup) I think the Core BC policy → (which I’m also trying to follow as much as possible) permits adding a new method. Therefore, I think we should add \Stringable directly there.

However, it seems that the same is true for several other interfaces:

• FieldInterface
• ItemInterface
• TextTokenInterface
• TextValueInterface
• ConditionInterface
• QueryInterface
• ResultSetInterface

I therefore think we should add \Stringable to those as well – or do you see a reason why not?

I added this to your MR (closed the other, as you said) and also took the opportunity to add the string return typehint throughout. (It seems this is not required, but still good to do, I think.)

Please tell me what you think. And thanks again, in any case!

Probably should still have a change record, though?

@drunken monkey, thanks for the feedback and input!

I'm still new to maintaining modules in the Search API ecosystem, so I'm not sure I'm qualified enough to have an opinion on whether or not FieldInterface, ItemInterface, TextTokenInterface, TextValueInterface, ConditionInterface, QueryInterface, and/or ResultSetInterface should implement \Stringable. In elasticsearch_connector, we only explicitly try to convert ConditionGroups to strings.

It seems logical that all of these things should implement \Stringable — and from what I know of both ElasticSearch and Solr, all these things get converted to strings in some way, because both engines use JSON as an data interchange format — but I can also imagine a search backend that uses a binary data interchange format, where that would make less sense (in fact, I worked with one briefly in my pre-Drupal career) — but I dunno how useful that would be as a Drupal search backend, and I suppose if you really had to conform to the \Stringable interface in that case, then you could base64-encode the binary data, or return an empty string, etc.

I do like adding the : string return typehint to function __toString().

Thanks for your feedback on this.

Thinking about this some more, though, I’m not sure I want to add that interface after all. The thing is, the string representation for all of these classes (except for TextTokenInterface and TextValueInterface) is just meant for debugging purposes. For instance, it allows the Views preview to show something akin to the SQL query that is displayed for normal views. If you are relying on this for any kind of logic, I’d say you’re doing it wrong and a code style check would be right in flagging this as an error.

I only now looked at the code you linked that does use ConditionGroup::__toString() and this should definitely check whether ConditionGroup::getConditions() is empty instead. The string representation for an empty condidition group could, in theory, change at any point without warning.

We could even talk about adding an isEmpty() method to ConditionGroupInterface to make this completely clean. Though it seems you could also just remove the check entirely and rely on the foreach loop being a no-op if the condition group doesn’t contain any conditions? (You’d just need to be aware that $filters might be empty at the end of the method in that case.)

Adding the \Stringable interface to me would seem to signify that the string representation of the class is something that you can rely on in any way, which is definitely not the case.

So, I think I’m now more in favor of adding the interface to just TextTokenInterface and TextValueInterface, and to add the string return typehints.
What do you think? Does my reasoning make sense, in your opinion?
And would you want to add an isEmpty() method for condition groups? (Would probably be a separate issue, though.)

@drunken monkey, I apologize for the delay in replying: thank you for your patience!

Taking a fresh look at it, I think your reasoning for not wanting to add \Stringable to ConditionGroupInterface or ConditionGroup makes a lot of sense, and I agree with you.

I will adjust my code as you suggested...

-if (!empty($condition_group->__toString())) {
+if (!empty($condition_group->getConditions())) {

I agree that a isEmpty() function would be ideal: it makes the code much easier to understand at-a-glance...

if (!empty($condition_group->__toString())) {
if (!empty($condition_group->getConditions())) {
if (!$condition_group->isEmpty()) {

... I think this should be done in a separate ticket though.

I support adding \Stringable to TextTokenInterface and TextValueInterface, and to add the string return typehints in a separate ticket.

Thank you very much!

Now that I've created the follow-up issues (commit 2af064d, ✨ FilterBuilder: change how we determine if a condition group is empty Fixed , ✨ Add isEmpty() function to ConditionGroupInterface Active , ✨ Declare that TextTokenInterface and TextValueInterface implement \Stringable Needs review ) I'm going to close this issue as "Won't fix"

Thanks again, @drunken monkey!