Posted by sun on July 22, 2011 at 3:36pm
9 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API change, needs backport to D7, Needs issue summary update |
Issue Summary
@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.)
Change records for this issue
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal.db-not-equal.0.patch | 7.92 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 33,584 pass(es). | View details | Re-test |
Comments
#1
Nice, just going to toss out a +1 on this.
#2
If the bot is happy I'd say this is RTBC.
#3
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.
#4
This is backward compatible, so +1 from me.
#5
+++ 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.
#6
Speaking of runtime checks...
#7
Alright, test results prove that none of the trigger_error()s were triggered, so this patch does convert all != into <>.
#8
We need to insure backward compatibility of the
<>operator in EntityFieldQuery, at least for Drupal 7.#9
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.
#10
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.#11
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.
#12
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?
#13
Yes let's do that.
#14
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()#15
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.
#16
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.
#17
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.
#18
@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.
#19
@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.
#20
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)
#21
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.
#22
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.
#23
Tagging.
#24
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.
#25
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).
#26
<?phppublic 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.#27
Thanks, changed accordingly.
#28
#27 looks better yeah, I had forgotten about people writing EFQ queries...
#29
#28 could've marked this RTBC but apparently I didn't.
#30
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.
#31
#32
And since adding that summary is relatively easy, tagging as Novice.
#33
Tried to fix this
http://drupal.org/node/1339664
#34
I added a bit more detail to the change notice.
#35
#36
Automatically closed -- issue fixed for 2 weeks with no activity.
#37
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.
#38
Note that 8.x is different in this regard and needs no patch.
#39
Blargh, stupid patch uploaded. Try this.
#40
I think this is normal now and not major/critical.
#41
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.)
#42
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.
#43
Nonetheless we gain simplicity by the patch.
#44
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.
#45
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.