@catch reminded me of this in #1164852-65: Inconsistencies in field language handling, and it turned out that core uses the wrong operator all over the place.

Attached patch is a quick fix. Note that I only grepped for '!='. I don't think there are any instances of "!=", but there may very well be instances of != in plain db_query*() SQL strings.

For D8+ (separate issue), we should remove support for '!=' from EFQ and also throw an exception in DatabaseCondition::condition() in case $operator == '!=' is passed. (No magic conversion -- otherwise, people won't understand this ever.)

Files: 
CommentFileSizeAuthor
#39 1226796.patch2.04 KBchx
PASSED: [[SimpleTest]]: [MySQL] 41,226 pass(es).
[ View ]
#37 1226796.patch2.02 KBchx
FAILED: [[SimpleTest]]: [MySQL] 38,873 pass(es), 151 fail(s), and 58 exception(s).
[ View ]
#27 drupal.db-not-equal.27.patch14.17 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,911 pass(es).
[ View ]
#25 drupal.db-not-equal.25.patch14.6 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,877 pass(es).
[ View ]
#24 operator.patch9.76 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,859 pass(es).
[ View ]
#17 operator.patch14.76 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,802 pass(es).
[ View ]
#7 drupal.db-not-equal.7.patch10.04 KBsun
PASSED: [[SimpleTest]]: [MySQL] 33,577 pass(es).
[ View ]
#6 drupal.db-not-equal.6.patch10.89 KBsun
PASSED: [[SimpleTest]]: [MySQL] 33,576 pass(es).
[ View ]
#5 drupal.db-not-equal.5.patch10.04 KBsun
PASSED: [[SimpleTest]]: [MySQL] 33,578 pass(es).
[ View ]
drupal.db-not-equal.0.patch7.92 KBsun
PASSED: [[SimpleTest]]: [MySQL] 33,584 pass(es).
[ View ]

Comments

neclimdul’s picture

Nice, just going to toss out a +1 on this.

plach’s picture

If the bot is happy I'd say this is RTBC.

catch’s picture

Looks good to me as well, although I wonder a bit if there was a design decision to use != in EntityFieldQuery. Sticking with ANSI seems fine in that case too though.

Damien Tournoud’s picture

This is backward compatible, so +1 from me.

sun’s picture

Status:Needs review» Needs work
StatusFileSize
new10.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,578 pass(es).
[ View ]
+++ b/modules/field/tests/field_test.storage.inc
@@ -287,6 +287,9 @@ function field_test_field_storage_query($field_id, $conditions, $count, &$cursor
+            if ($operator == '!=') {
+              $operator = '<>';
+            }

This doesn't seem to be required after all:

> php -r "$a = 1; $b = 2; var_dump($a <> $b);"
bool(true)

> php -r "$a = 1; $b = 1; var_dump($a <> $b);"
bool(false)

I've to admit I didn't know that "<>" is a valid operator in PHP, too.

Also grepped some more for " != " in plain SQL queries. Had to try several regular expressions to find various different instances. Catching all is most probably only possible at runtime.

5 days to next Drupal core point release.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new10.89 KB
PASSED: [[SimpleTest]]: [MySQL] 33,576 pass(es).
[ View ]

Speaking of runtime checks...

sun’s picture

StatusFileSize
new10.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,577 pass(es).
[ View ]

Alright, test results prove that none of the trigger_error()s were triggered, so this patch does convert all != into <>.

Damien Tournoud’s picture

We need to insure backward compatibility of the <> operator in EntityFieldQuery, at least for Drupal 7.

sun’s picture

Issue tags:+needs backport to D7

I originally considered that too and actually thought we had some magic code somewhere that would automagically convert a "!=" condition into a "<>" condition, but I was utterly wrong with that assumption - there is no such conversion anywhere.

Therefore, neither db_query(), nor db_select(), nor EFQ actually support the "!=" operator currently. The != operator might be supported by an alternative field storage engine than SQL, but in that case, that field storage engine needs to implement support for it. Since you don't know which field storage engine will be called for an EFQ, you have to use the <> operator.

All code that currently uses the != operator should actually not work properly with other SQL database drivers than MySQL currently, since, again, there's no conversion into <> in core currently.

This patch only corrects the EFQ documentation accordingly.

Damien Tournoud’s picture

Therefore, neither db_query(), nor db_select(), nor EFQ actually support the "!=" operator currently. The != operator might be supported by an alternative field storage engine than SQL, but in that case, that field storage engine needs to implement support for it. Since you don't know which field storage engine will be called for an EFQ, you have to use the <> operator.

Right for db_query() and db_select(). Those never pretended to support != which is *not* ANSI SQL.

Wrong for EFQ. != was explicitly documented as a valid operator. As a consequence, we need to continue supporting it in Drupal 7, even if we change the documentation to promote the <> operator.

catch’s picture

So the EFQ changes:

- field_sql_storage just passes through whatever condition is given to it, so doesn't care if it's != or <>

- MongoDB however does this:

    case '!='          : return array('$ne' => $value);

And does not account for <>.

So the documentation is actually the API.

Patch is fine for D8 I think (unless someone objects to changing to the ANSI operator for EFQ). But for D7 it's an API change.

Two things we could do:

1. Have field_sql_storage() internally convert != to <>, that would be a straight bug fix for D7.

2. In addition to #1, add support for <> to EFQ, and have core convert that to != before sending it to query backends. Could then mark != itself as deprecated. (Yes this means we'd be converting <> to != then back to <> again...)

If any of this is controversial we might want to split it off to a separate issue compared to the straight bug fixes here.

sun’s picture

It seems to me we have a general agreement on D8 (remove != in favor of <>; i.e., current patch), so let's get that in first, afterwards discuss the backport?

catch’s picture

Status:Needs review» Reviewed & tested by the community

Yes let's do that.

Damien Tournoud’s picture

For Drupal 7 I'm more concerned about the users of the EFQ API then for implementations of the Field Storage API. I'm totally fine breaking the latter (after all, there are like two or three implementations), but not fine at all breaking the former.

Here is what I suggest:

* We remove any mention of != in the documentation of EFQ
* but we still continue supporting != by converting it to <> in the various ->*Condition()

webchick’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs change record

Ok, committed #7 to Drupal 8. This needs an API change node created.

Moving down to 7.x for further discussion. FWIW, I agree that we can't remove support for != in D7, but I'm fine deprecating it.

neclimdul’s picture

I don't see a reason to remove support for it. Just removing use of it seems good. We should be setting a good example where ever possible and there shouldn't be any real impact to the backport.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new14.76 KB
PASSED: [[SimpleTest]]: [MySQL] 35,802 pass(es).
[ View ]

Here's a start on the approach in #11.

Docs change to '<>'.

That operator is changed back to '!=' internally to pass to field engines.

The SQL helpers then change '!=' back to '<>' again so it's ANSI SQL.

This is not pretty, but should be backwards compatible for both EntityFieldQuery API users, and field storage engines themselves.

Ran those two tests but nothing else, the rest of the patch applied cleanly.

Damien Tournoud’s picture

@catch: any particular reason not to just follow #14? Seriously, there might be two or three implementation of the Field Storage API out there. I'm not too worried about the API change on that side.

catch’s picture

@Damien, I don't see any reason to introduce an API change if we don't have to, but I also wouldn't object to a patch that does what you suggested in #14 either.

sun’s picture

Issue tags:+API change

I'd also go with the proposal in #14. All of the existing field storage engines in contrib are from core developers anyway...

http://drupal.org/project/devnull (@catch)
http://drupal.org/project/riak_field_storage (@dmitrig01)
http://drupal.org/project/pbs (@bjaspan)
http://drupal.org/project/mongodb (@Damien Tournoud, @chx)

(btw, we could really use a "[Field] Storage engine" project/module category for all field storage engines and mayhaps also database drivers... the existing Database drivers category is almost empty)

neclimdul’s picture

Only one of those even has a release. I'd love to see movement on PBS but isn't it just generally accepted to be dead by now?

I think I might be a little unsure of the code being debated but on a just overall view, I'm leery of any API changes though. Its not just contrib we're supporting. The random site out there with a entirely custom field backend who can't have downtime suddenly has to develop a change. Sites using MongoDB have to possibly test an upgrade path and upgrade a contrib module with the core update or get weird behavior.

I'll trust Damien and others review of this because they have a much deeper understanding of the field system which I have little experience with but it does make me nervous.

catch’s picture

Sites using MongoDB have to possibly test an upgrade path and upgrade a contrib module with the core update or get weird behavior.

Yeah I think this is valid. MongoDB does this with operators that aren't valid:

    default : throw new EntityFieldQueryException("$operator not implemented");

Core itself is not using EntityFieldQuery yet, but it's possible that a contrib module does, and that starts using the <> operator before MongoDB module is upgraded.

xjm’s picture

Tagging.

catch’s picture

StatusFileSize
new9.76 KB
PASSED: [[SimpleTest]]: [MySQL] 35,859 pass(es).
[ View ]

Here's one with #14. The API change is that storage backends need to add support for <> if they only support !=.

I did not make field_sql_storage internally convert != to <>, we could do that if we really wanted to.

sun’s picture

StatusFileSize
new14.6 KB
PASSED: [[SimpleTest]]: [MySQL] 35,877 pass(es).
[ View ]

Attached patch implements what I think has been the proposal in #14.

i.e., EFQ internally converts != to <>, but field storage engines are otherwise untouched (and need to implement support for the new operator).

Damien Tournoud’s picture

Status:Needs review» Needs work
<?php
  
public function addCondition(SelectQuery $select_query, $sql_field, $condition, $having = FALSE) {
+   
// Convert the '!=' operator to its ANSI SQL equivalent.
+    if ($condition['operator'] == '!=') {
+     
$condition['operator'] = '<>';
+    }
?>

This one is not needed (it's an internal function).

<?php
--- a/modules/field/tests/field_test.storage.inc
+++ b/modules/field/tests/field_test.storage.inc
@@ -283,6 +283,7 @@ function field_test_field_storage_query($field_id, $conditions, $count, &$cursor
             $match
= $match && $row->{$column} == $value;
             break;
           case
'!=':
+          case
'<>':
?>

It think we want to remove != from here.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new14.17 KB
PASSED: [[SimpleTest]]: [MySQL] 35,911 pass(es).
[ View ]

Thanks, changed accordingly.

catch’s picture

#27 looks better yeah, I had forgotten about people writing EFQ queries...

catch’s picture

Status:Needs review» Reviewed & tested by the community

#28 could've marked this RTBC but apparently I didn't.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Ok, committed and pushed #27 to 7.x.

We still need that API change notice. Also, for the various field storage engines, reaching out via contact form to those maintainers and pointing them there would also be greatly appreciated. I've made a note to include it in the release notes of 7.9 as well.

webchick’s picture

Title:Not equal operator '!=' is not supported by all databases, must be '<>'» API change notice for Not equal operator '!=' is not supported by all databases, must be '<>'
Category:bug» task
Priority:Major» Critical
Status:Needs work» Active
webchick’s picture

Issue tags:+Novice

And since adding that summary is relatively easy, tagging as Novice.

aspilicious’s picture

Status:Active» Needs review
xjm’s picture

Status:Needs review» Fixed

I added a bit more detail to the change notice.

xjm’s picture

Title:API change notice for Not equal operator '!=' is not supported by all databases, must be '<>'» Not equal operator '!=' is not supported by all databases, must be '<>'
Issue tags:-Novice, -Needs change record

Status:Fixed» Closed (fixed)

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

chx’s picture

Status:Closed (fixed)» Active
StatusFileSize
new2.02 KB
FAILED: [[SimpleTest]]: [MySQL] 38,873 pass(es), 151 fail(s), and 58 exception(s).
[ View ]

The public facing methods of EntityFieldQuery are not doing any data manipulation. It is collecting data and is not doing anything else. Take your SQL problems to the SQL-talking parts. Also, the attached patch is also like a dozen lines of code shorter. Code reuse FTW.

chx’s picture

Note that 8.x is different in this regard and needs no patch.

chx’s picture

Status:Active» Needs review
StatusFileSize
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 41,226 pass(es).
[ View ]

Blargh, stupid patch uploaded. Try this.

aspilicious’s picture

Priority:Critical» Normal

I think this is normal now and not major/critical.

chx’s picture

Priority:Normal» Critical

Can't see how a critical breaking my module is not critical to fix. (Oh and of course I was able to fix this in contrib too. But still.)

catch’s picture

Priority:Critical» Normal
Status:Needs review» Fixed

It's not critical, the patch was committed with the API change well known in advance (I was one of the people who objected to it), and it's now been in core for more than six months.

The fact this broke field storage engines should have been in the change notice, and since mongodb is the only viable one at the moment someone should have directly pinged you to warn, but http://drupalcode.org/project/mongodb.git/blobdiff/d55d5186a0f8598cfedb8... has already fixed it as well. Moving back to fixed.

chx’s picture

Title:Not equal operator '!=' is not supported by all databases, must be '<>'» Simplify not equal operator '!=' is not supported by all databases, must be '<>'
Status:Fixed» Needs review

Nonetheless we gain simplicity by the patch.

Damien Tournoud’s picture

Status:Needs review» Fixed

I'm not sure what the fuss is all about. This patch just enforced the <> operator for in Field Storage API. Entity Field Query accepts both forms for backward compatibility, but any implementation of the Field Storage API should only accept <>.

Doing this in the data collection object is correct and there is nothing to fix here. Yes, we broke the Field Storage API, but that was six month ago and I don't see how that can possibly be a big deal.

chx’s picture

Status:Fixed» Needs review

Meh. Even by principle it is wrong to do anything with the data colected in the EntityFieldQuery methods. That's partially what the fuss is about the other part is, why did this go in without anyone pinging me.

mgifford’s picture

39: 1226796.patch queued for re-testing.

mgifford’s picture

Issue summary:View changes

Still applies. Do we want to bring in the patch in #39?