Simplify not equal operator '!=' is not supported by all databases, must be '<>'

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

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

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.

This is backward compatible, so +1 from me.

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

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.

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.

Yes let's do that.

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()

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.

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.

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.

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

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

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.

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.

Tagging.

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.

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

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

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

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

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.

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

Tried to fix this

http://drupal.org/node/1339664

I added a bit more detail to the change notice.

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.

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

Blargh, stupid patch uploaded. Try this.

I think this is normal now and not major/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.)

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.

Nonetheless we gain simplicity by the patch.

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.

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.

39: 1226796.patch queued for re-testing.

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