Operator 'Is not equal' of BooleanOperator doesn't work

I'll try and see if I can manually test this issue.

I've manually tested this patch and it does appear to effect the advertise change.

Steps taken.

drush generate-content 20 --types=page,article
Manually unpublished 10 nodes.
Manually edited the "Content" view.

• remove published content or admin filter
• remove published content (grouped) filter
• Added new published content, setting configuration as demonstrated in screenshot.

Visual results attached.

Well, RC eligible means for example documentation/test only changes, this is not the case here. Let's add 'rc target triage' instead.
Do you mind describing in the issue summary what 'doesn't work mean'? Is it not filtered at all, or do we see no items?

Yes, I think this could be fixed in a patch release instead of 8.0.0, but could also make it into 8.0.0 because it has value and no cost.

Sure, but its the point of the rc target thing to focus on what is really important.

This fixes a bug, so we should IMHO allow that.

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -213,25 +230,36 @@ public function query() {
+   *   (optional) Either static::EQUAL, either static::NOT_EQUAL. Defaults to

either... or

Fixes #14.

+++ b/core/modules/views/src/Plugin/views/filter/BooleanOperator.php
@@ -213,25 +230,36 @@ public function query() {
     else {
       if (!empty($this->definition['use_equal'])) {
-        $this->query->addWhere($this->options['group'], $field, 1, '=');
+        $this->query->addWhere($this->options['group'], $field, 1, $query_operator);
       }
       else {
-        $this->query->addWhere($this->options['group'], $field, 0, '<>');
+        $reverse_operator = $query_operator == static::EQUAL ? static::NOT_EQUAL : static::EQUAL;
+        $this->query->addWhere($this->options['group'], $field, 0, $reverse_operator);
       }

The double negative here really confuses me - could use a comment at least?

Also what's the likelihood that someone extends BooleanOperator?

Also what's the likelihood that someone extends BooleanOperator?

Is there more to boolean than boolean?

Also what's the likelihood that someone extends BooleanOperator?

It's possible that there is a field with extra data and the developer wants the query logic to be modified by some extra data. Probably something crazy like that.

From #16:

Also what's the likelihood that someone extends BooleanOperator?

Very high... We do it in core! :)

See core/modules/user/src/Plugin/views/filter/Current.php

The double negative here really confuses me - could use a comment at least?

Agreed. In rereading the code, I don't think it's necessary. I've also cleaned up the adminSummary and queryOpBoolean function signature.

That looks much better.

Given thus does get extended, do we need to be concerned about the change to the signature of queryOpBoolean? Given the extra argument has a default value, calling it's going to be fine, so it's only if it was overridden that it could be a problem.

Can think of ways to make the change without the theoretical bc break, but in this case it might just be OK to do it.

@catch, got it -- now I understand the concern about BooleanOperator being inherited. Didn't think of this as a BC break, but you're right that it would be...

In core, there's just the one filter plugin that inherits BooleanOperator. But who knows what's in contrib? DrupalContrib.org doesn't show any usages, but it's got pretty limited coverage.

So, is there wiggle room in the allowed changes for D8? If not, I can add a BC layer and mark it for removal in D9.

flag seems to have a class that extends from that, for example. That's the only one outside of core that I have in my project.

flag seems to have a class that extends from that

Based on that, we need a BC layer. I'll work on one shortly.

Quick shot at a BC layer before I have to run out the door...

Edit: I should say I haven't tested this with Flag yet.

@mikeker we can make the original change fine in a minor release because protected methods are documented as @internal. We expect extenders of non-base classes to keep up, and in practice few or none would actually be affected.

However we're also trying not to change even @internal things in patch releases. So one option here would be to fix this without the helper, then have an 8.1.x follow up to add it. Or your patch with the new helper documented as @internal and with a leading underscore, which we then revert to the original patch in 8.1.x. This isn't a problem we've had before in the same way.

OK discussed with @xjm.

This is one of several issues where:

- the patch is fine for a minor version
- due to semver and our own approach to not breaking @internal things in patch releases, it's not fine for a patch release, but there ought to be simple enough way to do it in a patch-safe way
- requiring the patch-safe patch first, means we end up CNW-ing a patch that's fine for 8.1.x, then we have a patch to both branches, then another follow-up to get us back to where the original patch would have in 8.1.x which is double or triple the work.
- on top of that, the actual disruption/breakage from breaking semver here is very likely to be zero, but I think there's benefits to being strict on this, since as we rack up hundreds/thousands of commits to patch releases, we'll eventually hit a 'winner' the looser we are.

Trying this:

- moving this issue to 8.1.x
- RTBC for the patch in #19
- once that's committed, we can move it back to 8.0.x and 'to be ported' for the patch-safe fix.

Worst case, it never gets backported, everyone gets the fix in April when 8.1.0 is out anyway, which is better than the issue getting completely stuck on semver discussions.

OK, committed/pushed to 8.1.x, thanks!

Moving to 8.0.x for backport. Since #25 is a start on that, moving to CNW based on #26's review.

Or your patch with the new helper documented as @internal and with a leading underscore,

For me that's the easiest way to do this in a patch-safe way.

If the 8.1.x commit broke BC, then does this need a change record?

@mradcliffe, I suppose it does. Draft CR is here: https://www.drupal.org/node/2635864. Feel free to publish if you feel it covers this issue. Thanks.

@catch, Thank you for thinking through the complicated contortions needed to get this into D8 as soon as possible. Much appreciated!

Or your patch with the new helper documented as @internal and with a leading underscore

I believe this is what you're asking for...

Seems a wrong indent?

Yeah, must've fat-fingered the tab key at some point... Fixed.

Committed/pushed the backport to 8.0.x, thanks!

I could be mistaking the purpose of the 'accept_null' field in the views_data_definition, but I believe this has introduced a regression whereby "Is not equal to" used in conjunction with accept_null breaks.

SQL before the patch:

> AND( (some_bool <> '0') AND (some_bool IS NOT NULL ) )

SQL after the patch:

> AND( (some_bool <> '0') OR (some_bool IS NULL ) )

I don't know enough about the data definitions to say which result is correct and the accept_null is barely used in core, so it's going to be annoying to come up with concrete test cases.

I appear to still have this issue - or another that shows the same symptoms - in 8.7.6?
Specifically, setting 'is not equal to' doesn't change the operator from = to !=

#3073518: 'Is not equal to' operator ignored on exposed, grouped filter