Synopsis
When using EntityFieldQuery::entityCondition() to add conditions, the fully qualified column names are not used for certain conditions. Therefore, when the function _node_query_node_access_alter
in modules/node/node.module
rewrites the query, it is possible to have ambiguous column names used in the conditions.
Example
An example is provided by using the entityreference module alongside the domain module. The bug filed with the entityreference module is here #1309704: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052.
The error returned is as follows:
Integrity constraint violation: 1052 Column 'nid' in where clause is ambiguous: SELECT DISTINCT node.nid AS entity_id, node.vid AS revision_id, node.type AS bundle, :entity_type AS entity_type FROM {node} node INNER JOIN {node_access} na ON na.nid = node.nid WHERE (nid IN (:db_condition_placeholder_0)) ....
Since entityCondition did not use the fully qualified column name, the column nid is ambiguous.
Possible fix
Here is a patch.
--- entity.orig.inc 2011-10-28 23:58:27.000000000 -0400
+++ entity.inc 2011-10-28 23:58:32.000000000 -0400
@@ -1079,7 +1079,7 @@
$id_map['entity_id'] = $sql_field;
$select_query->addField($base_table, $sql_field, 'entity_id');
if (isset($this->entityConditions['entity_id'])) {
- $this->addCondition($select_query, $sql_field, $this->entityConditions['entity_id']);
+ $this->addCondition($select_query, "$base_table." . $sql_field, $this->entityConditions['entity_id']);
}
// If there is a revision key defined, use it.
@@ -1087,7 +1087,7 @@
$sql_field = $entity_info['entity keys']['revision'];
$select_query->addField($base_table, $sql_field, 'revision_id');
if (isset($this->entityConditions['revision_id'])) {
- $this->addCondition($select_query, $sql_field, $this->entityConditions['revision_id']);
+ $this->addCondition($select_query, "$base_table." . $sql_field, $this->entityConditions['revision_id']);
}
}
else {
Comment | File | Size | Author |
---|---|---|---|
#116 | efq-prefix-1325628-116-TESTS-ONLY.patch | 1.78 KB | David_Rothstein |
#116 | efq-prefix-1325628-116.patch | 4.39 KB | David_Rothstein |
#113 | efq_prefix-132562-100-d7-tests.patch | 1.8 KB | xjm |
#113 | efq_prefix-132562-100-d7-combined.patch | 4.4 KB | xjm |
#107 | efq_prefix-132562-100-d7-do_not_test.patch | 4.4 KB | xjm |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks for opening this. Triaging (bugs get fixed in 8.x first).
I think this type of issue probably apply to all the property conditions. We should try to fix all of those.
Comment #2
xjmAlso, no actual patch file here yet (and it would need to be rerolled in any case on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades).
Tagging as novice for someone to make a current patch using the proposed fix for all these conditions, as Damien suggests above.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedAt the minimum I think we need this.
Two things need to be checked:
field_sql_storage_field_storage_query()
doesn't seem to be on par withEntityFieldQuery::propertyQuery()
with regard to support for entities without a bundle... we need to check thisComment #5
xjmUntagging.
Comment #6
damien_vancouver CreditAttribution: damien_vancouver commentedI encountered this error while using node access modules and trying to update a node.
I backported the patch from #3 to 7.x, and it fixed the error for me (yay!).
I was going to take on the 8.x patch, but the first thing I notice is that entity.inc is gone from its old location. It appears Entity is now a core module, and I assume the code in question has moved into it. I'll work on a new one, once I track down the offending code's new location. I am not yet sure how to reproduce the error in a test, but I'll work on that too! Finally, I don't know the answer to either question posed in #3. I thought the SQL one would be easy to look up, but after wallowing through the SQL-92 standard a bit, it's not.
So, lots to do still. Meanwhile this PDO error has to go.... I've attached my working patch for Drupal 7. It will fail the test bot as this issue is still 8.x-dev, but it may fix the problem for you as it did me.
[ EDIT : Please use the patch from #7 below. This one has some extraneous file mode lines ]
Comment #7
damien_vancouver CreditAttribution: damien_vancouver commentedSorry for the spam. Please use this version of the Drupal 7.x patch instead of #6's. Some extra permission lines snuck into that version.
Comment #8
pjcdawkins CreditAttribution: pjcdawkins commentedThe patch in #7 works for me in Drupal 7.10.
[edit: and the same patch also works in Drupal 7.11]
Comment #9
dddbbb CreditAttribution: dddbbb commented#7 works for me too (Drupal 7.8). Many thanks.
Comment #10
clashar CreditAttribution: clashar commented#7 worked for me as well (D 7.8)
Comment #11
damien_vancouver CreditAttribution: damien_vancouver commentedI located the offending code in the new 8.x core entity.module. It is in core/modules/entity/entity.query.inc, and otherwise unchanged.
Here is a patch against 8.x. It's the same as before, except I added the "$base_query." prefix to the bundle query as well - as it was still missing in previous versions of the patch....
ie.
So - 8.x patch attached, and setting to "needs review" to see how the test bot likes it. There is still no simpletest yet. See below.
Also, here is a researched answer to the SQL question posed in #3:
Yes it is standard to ORDER BY an alias, because you order by a <column name> not a <table name>. From the ANSI SQL-92 specification, section 20.2 :
Then the definition of <order by clause> also points back to a <column name>
Throughout the spec, the <column name> is defined as a column identifier, not a base table. The base table name is exposed as the <column name> in a table reference, if an alias isn't specified. From section 6.3, Table Reference:
... All these point to yes, though it's hard to be sure wading through that standard. (I'd kill for a hyperlinked HTML version, but could only find RFC style plain text). On top of this though, I can speak from personal experience that it works in: MS-SQL, Sybase, Oracle, MySQL, PostGRES, and SQLite. I have used it in commercial projects on all of those platforms.
Next steps:
- We need a simpletest... but how exactly? It might be very tricky to write one for 8.x, without an abundance of contrib modules to test with (and besides, how would we test that in a simpletest?) The error is triggered in the wild for folks with multiple node_access modules turned on, and even then it is intermittent. Can anyone make any suggestions as to the best approach for writing a simpletest of that? I don't even see an easy way to functionally test just EntityFieldQuery:propertyQuery, as it's a protected method. I could really use some suggestions on how to proceed!
- We need to answer the remaining question from #3 above. I looked at the field_sql_storage_field_storage_query() code a bit but don't really understand what is wrong / different. Was that maybe the change with invalidating the query if the bundle was missing, from #3's patch?
As shown in this post, this error is happening in the wild for Drupal 7 people. (If you are one of these Drupal 7 people, please try the patch from #7 and report back here if it works for you). It'd be nice to light a fire under this, so we can get it fixed in D7 ASAP.
Comment #13
wipeout_dude CreditAttribution: wipeout_dude commented#7 Solved the issue for me on Drupal 7.10. Thank you damien_vancouver..
I am running Drupal 7.10 with Entity Reference and Content Access..
When a non-superuser tried to create content containing and entity reference field the following error was returned..
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'nid' in where clause is ambiguous: SELECT DISTINCT node.nid AS entity_id, node.vid AS revision_id, node.type AS bundle, :entity_type AS entity_type FROM {node} node INNER JOIN {node_access} na ON na.nid = node.nid WHERE (nid IN (:db_condition_placeholder_0)) AND (type IN (:db_condition_placeholder_1)) AND(( (na.gid = :db_condition_placeholder_2) AND (na.realm = :db_condition_placeholder_3) )OR( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) )OR( (na.gid = :db_condition_placeholder_8) AND (na.realm = :db_condition_placeholder_9) ))AND (na.grant_view >= :db_condition_placeholder_10) ; Array ( [:db_condition_placeholder_0] => 108 [:db_condition_placeholder_1] => childcare_setting [:db_condition_placeholder_2] => 0 [:db_condition_placeholder_3] => all [:db_condition_placeholder_4] => 2 [:db_condition_placeholder_5] => content_access_author [:db_condition_placeholder_6] => 2 [:db_condition_placeholder_7] => content_access_rid [:db_condition_placeholder_8] => 5 [:db_condition_placeholder_9] => content_access_rid [:db_condition_placeholder_10] => 1 [:entity_type] => node ) in EntityFieldQuery->execute() (line 1136 of C:\wamp\www\d7.10-dev\includes\entity.inc).
After applying the patch in #7 the issue is resolved and no other adverse effects noted..
Any chance of this patch making it onto Drupal 7.11??
Comment #14
wipeout_dude CreditAttribution: wipeout_dude commentedUpdate to my post at #13..
The patch at #7 breaks the ability to create "user" references with Entity Reference.. The lookup of "user" entities appears to fail in the widgets..
Comment #15
BTMash CreditAttribution: BTMash commentedThere is some other issue going on since the simpletests for core are failing. I'm trying to understand what is happening in the simpletest so atleast the patch passes on one side. For the rest of the simpletests...at the very least, providing screenshots of the bad behavior followed by screenshots of the good behavior after the patch is applied atleast gets us somewhere. I'll try and see if anything from the examples module's node access could be used in creating tests to see the behavior works. We could simply have two node access modules enabled, have the simpletest go to a page that is supposed to have X, Y content or show this error and if the error comes up then fail it.
Comment #16
damien_vancouver CreditAttribution: damien_vancouver commented@BTMash,
But are there ANY node access modules available to us in 8.x? (let alone two?) Aren't those all contrib? We have to make the simpletest work for 8.x-dev first in order to fix it in HEAD then backport, a la standard procedure.
We also need to test/see what's up with user reference being broken a la #14 above too. Meh!
Comment #17
BTMash CreditAttribution: BTMash commentedWe have atleast one in contrib to test against (node_access_example) for 8.x. However, I did not mean to make the work that needs to be done sound trivial (as you said, there are very few modules for 8.x let alone node access modules so trying to test this would be impossible at this point and the issue will remain open for a long time). I was only trying to say that screenshots (atleast from the 7.x version) could help in testing as well (seeing what errors pop up, trying to replicate the issue, etc as currently I am not entirely sure on the process to replicate the issue on my computer aside from running the simpletest in 8.x on my computer that is causing the fail; I'm more than happy to document this out).
Comment #18
damien_vancouver CreditAttribution: damien_vancouver commented@BTMash, yeah I totally agree, wasn't suggesting you thought it was trivial or anything! More lamenting that it might be really hard :) I agree about the screenshots too, or even fixing by inspection when tests can't be done. The part stumping me about the simpletest is how you go about creating a node access module and turning it on inside the test. I'm assuming this has been done somewhere in core.. otherwise how could those areas be tested? Maybe they aren't?
We could really use some input here from someone familiar with that area of testing core.. if any such person sees this. Otherwise I'm sure we can get some help on IRC, that was my backup plan.
Also, I totally agree on the screenshots front. This bug is so low level that it's probably going to have crazy side effects... those screenshots are a great start towards us figuring out how it explodes in the real world (which is really what matters here!)
I have less than zero time available until the end of this month, but I can allocate some more to chasing / fixing this after Feb 1st. Meanwhile, Screenshots of before and after error messages would be very helpful to post to this thread, as BTMash says!
Comment #19
BTMash CreditAttribution: BTMash commentedI have *some* level of familiarity with writing out core tests (I still get tied up on things but am willing to put in time, regardless). To answer your question about adding modules to tests, its actually quite simple. You define it during the setUp process for a test class
That will ensure that testing_entity_access_module gets enabled/installed for the particular test.
Comment #20
damien_vancouver CreditAttribution: damien_vancouver commentedGreat! That will totally do the job, we can just define two test modules like that. Thanks for that info, I need that for a core OpenID bug I want to fix as well but also had no idea how to write a test for!
As for this issue, I'll put some time in after the 1st trying to create said test! And we must still verify / fix the entity user reference issue.
Comment #21
wipeout_dude CreditAttribution: wipeout_dude commentedFollowing on from my comment at #13, here is a screenshot of the error generated when using the "autocomplete" widget and a User Entity Reference Field with the patch at #7 applied..
Just for clarity I am running the entity_reference module along with the content_access module..
Hope its useful..
Comment #22
damien_vancouver CreditAttribution: damien_vancouver commentedI had convinced myself this problem was gone, I didn't see it after my upgrade... but my customer was able to set it off. :(
Here's the patch from #7 again, but for Drupal 7.12. I also added the one missing $base_table as described in my #11.
I still have to test entity user reference as per #17 and see why/if that's broken.
But... if #7 fixed it for you before, the attached should do it for you again.
If you're new to this thread and aren't comfortable with patches, save the attached "drupal_entity_patched_entity.inc_drupal_7.12-1325628-22.txt" over your includes/entity.inc and that should be the same as having applied the patch.
Comment #23
TravisCarden CreditAttribution: TravisCarden commentedLet's roll #22 against Drupal 8 and change the issue status so it gets tested. (I made the tiniest little coding style/PHP performance improvement.) I'm attaching a D7 version, too, for those who need it in the meantime.
Comment #24
squaretone CreditAttribution: squaretone commentedTravisCarden's patch (drupal-1325628-23-d7.patch) fixed the issue for me.
Comment #25
damien_vancouver CreditAttribution: damien_vancouver commentedHey look at that, it passed testing! I guess the test fails on #11 were due to other stuff broken in 8.x. :)
So.. do we need a simpletest now? What's next to get this in 8.x so we can get the backport applied to 7.x?
Comment #26
damien_vancouver CreditAttribution: damien_vancouver commented@wipeout_dude,
You have the only reported problem with the patch here, from your #14. I tried to reproduce but can't using the latest patch here (I used #22, but #23 is the same and both are the same as what you used in #13).
I tried to test like this:
- Added an Entity Reference field to a content type
- Set it to be a User reference
- Added a new node of this content type as a regular non-admin user
- Typed a few letters of a username into the autocomplete widget, it looked up the user with no ajax error as shown in screenshot from #21
- Selected the matching username
- Saved the node.
- no errors.
Please re-test and let us know if you're still having problems. I think maybe something else was going wrong for you, an unrelated bug.
@TravisCarden, Re: the performance improvement, I think the style used in the original patch was because the code was copied from the "Order the Query" logic around line 1260. That's the only other place I see it though:
If we're going to use the single quoted style, then I think we should change that one to match so it's consistent throughout the file? Want to add that to your patch?
Comment #27
wipeout_dude CreditAttribution: wipeout_dude commented@damien_vancouver -
Thanks for your time trying to solve this..
I have just tested on a fresh clean install and managed to repeat the ajax error, the error even happens as UID 1 (admin)..
Process..
1) Download D7.12 and install using standard install profile..
2) Patch entity.inc from #23
3) Download latest stable versions of entity_reference, ctools, devel, content_access and entity modules..
4) Enable Content Access, Entity Reference and Devel Generate..
5) Generate 50 users and basic pages with devel..
6) Create content type noderef and add an entity node reference field to basic pages..
7) Attempt to create content noderef, entity reference field works and shows selection of basic pages..
8) Create content type userref and add an entity user reference field..
9) Attempt to create userref, enter letter in entity reference field and attached ajax error is returned..
As I said this is with the Admin user so haven't got to the point of testing non-admin user at this stage..
Added a current screenshot in case anything in the error message is different in any way to the last one..
Comment #28
damien_vancouver CreditAttribution: damien_vancouver commented@wipeout_dude:
OK I have it happening now. I wasn't seeing it because I was doing it as a regular user, and didn't have the "administer" users permission. We definitely do have something going wrong with the patch code. I did some sleuthing:
If you look at the actual function that's blowing up, ie. line 289 of entitreference/handler/base.inc, line 289, you will see that it's inside a permissions check for "administer users", so you have to have that permission to trigger the error.
The error definitely goes away again when I roll back the patch to stock Drupal 7.12. Line 289 which is exploding is the third line here:
I put a breakpoint there to inspect $condition and see what's in there. The "field" key was the same in both cases (users.name). But PHP is not seeing one as a string?? This led me back to what had changed. Casting the $base_table prefixes etc. as strings didn't work.
Then I tried removing the logic added by Damien Tournoud in #3, regarding entities without a bundle, as I spotted a 1 = 0 in the WHERE clause and recognized it from the code there. When I took out the if (!empty($entity_info['entity_keys']['bundle'])) check and got a different 500 error. This time the query was failing with a "column not found: users.bundle". So, what is actually happening when we try user references is, the else block of this code from the patch fires:
Which is in turn causing entityreference to explode for users that have the "administer users' permission.
I still don't understand why we're invalidating the query here, but we are given a vital clue in #3:
My suspicion is we need to handle that else block better than "invalidate the query". Anyone got any deeper understanding of what is going wrong based on this, or suggestions?
setting status back to Needs Work until we can squash this.. we do appear to be breaking entity reference for users.
Comment #29
webmaster-eddie CreditAttribution: webmaster-eddie commentedJust wanted to add that I'm geting this error with multilingual forums (also enabled OG and entity reference - latest versions):
Drupal 7.12 core:
PDOException : SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'status' in where clause is ambiguous: SELECT f.tid AS tid, COUNT(n.nid) AS topic_count, SUM(ncs.comment_count) AS comment_count FROM {node} n INNER JOIN {node_comment_statistics} ncs ON n.nid = ncs.nid INNER JOIN {forum} f ON n.vid = f.vid INNER JOIN {node} node ON f.tid = node.tid WHERE (status = :db_condition_placeholder_0) AND (n.language IN (:db_condition_placeholder_1, :db_condition_placeholder_2)) GROUP BY tid; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => en [:db_condition_placeholder_2] => und ) dans forum_forum_load() (ligne 779 dans /var/aegir/platforms/Drupal7-12-WebSite1/modules/forum/forum.module).
Eddie
Comment #30
TravisCarden CreditAttribution: TravisCarden commentedThanks for the feedback, Eddie. It looks like you're rather new to Drupal.org. When you change project or version information in an issue comment, it changes it for the whole issue. So, since bugs are fixed in Drupal 8 first and then backported to 7, I'm changing the version back. Learn more about how the issue queues work. Welcome to the community!
:)
Comment #31
Xomby CreditAttribution: Xomby commentedI dug through some code throughout D7.12, and I see that in includes/common.inc, if the entity has no "bundle", it is assumed to be a single bundle named after the entity type (see includes/common.inc lines 7498-7510). This is an example of really handling the issue instead of just crapping the whole thing out.
By their logic,
$entity_type
is synonymous with$info['entity keys']['bundle']
and in includes/entity.inc we're working with$this->entityConditions['bundle']
which, while I can't figure out what the internal structure of that is, i'm guessing it should be similar or be a similar value...So, to test this theory I replaced (in version of includes/entity.inc patched with #22):
$select_query->where('1=0');
with
$this->addCondition($select_query, $sql_field, $entity_type, $having);
I can't get the error to come up anymore, but if someone else could test their setup, I'd be grateful.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedHi,
after applying #23 patch (d7.12) I still have a problem (screenshot in attachment). I am using EntityReference and Taxonomy Access Control module. Without patching when no-admin user try to add content I received error like this:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'nid' in where clause is ambiguous: SELECT DISTINCT node.nid AS entity_id, node.vid AS revision_id, node.type AS bundle, :entity_type AS entity_type FROM {node} node INNER JOIN {node_access} na ON na.nid = node.nid WHERE (nid IN (:db_condition_placeholder_0)) AND (type IN (:db_condition_placeholder_1)) AND(( (na.gid = :db_condition_placeholder_2) AND (na.realm = :db_condition_placeholder_3) )OR( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) )OR( (na.gid = :db_condition_placeholder_8) AND (na.realm = :db_condition_placeholder_9) ))AND (na.grant_view >= :db_condition_placeholder_10) ; Array ( [:db_condition_placeholder_0] => 370 [:db_condition_placeholder_1] => kontrahent [:db_condition_placeholder_2] => 0 [:db_condition_placeholder_3] => all [:db_condition_placeholder_4] => 2 [:db_condition_placeholder_5] => taxonomy_access_role [:db_condition_placeholder_6] => 4 [:db_condition_placeholder_7] => taxonomy_access_role [:db_condition_placeholder_8] => 10 [:db_condition_placeholder_9] => taxonomy_access_role [:db_condition_placeholder_10] => 1 [:entity_type] => node ) w EntityFieldQuery->execute() (linia 1136 z /[site adress]/includes/entity.inc).
Patch solve this problem but fields with entity reference are empty if I use select list widget or error (screenshot) if I use auto complete text filed widget.
Do you know any other solution?
Comment #33
damien_vancouver CreditAttribution: damien_vancouver commented@Xomby, I tried your suggestion from #31 but got the attached error (drupal-entity_comment31_error-1325628-33.png).
But your suggestion got me thinking.. how did it work for user reference before this patch's change? Sure enough, replacing that branch of code (the old where 1=0) with the original code from entity.inc works in all cases for my testing:
EDIT March 14th: Don't use these patches to fix this bug for D7, they are incorrect (See #35). The latest correct patch for this issue is the D7 version from #23.
So... new patches attached for D8 and D7! I incorporated TravisCarden's performance improvement (using single rather than double quotes) from his revisions in #23 (replacing "$base_table." with $base_table . '.' .). As I suggested in #26, I also fixed that in the two other places it appears in entity.inc so that the code is consistent throughout. (We may need to limit the patch just to the bug fix and not touch performance stuff. I'm not sure on the policy for that, if someone more informed could comment here if that's discouraged, and I'll remove the performance enhancement).
We still need simpletests
We should test both the original issue (multiple node access modules active for a node's content type causing an invalid query on node form validation, is how I trigger it), as well as the user reference auto-complete problem (see #13, #26, #27).
Other possible side effects and testing needed
Have we introduced other, similar side effects to those? Can anyone think of other things that aren't a bundle (like the user reference) we should/could test too? I'm not yet convinced yet that by leaving the original code we couldn't still trigger the original error, if we used some kind of user-based access module. I'll try and test that theory.
@Wipeout_dude, as you are the one having problems with the user reference on a real (not a test) site, your test results are probably the most important to hear about.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commented@damien_vancouver I have also working site and I have problem with user reference. Other references seems to be good. But I can't apply drupal-entity_fully_qualified_db_column_names_drupal7.12-1325628-33.patch like in Test result.
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe identified issue with Entity Reference fields targetting the User entity is a bug in Entity Reference. Please disregard it. The
1 = 0
from my original patch is correct and just takes code that is already inEntityFieldQuery::propertyQuery()
and applies it tofield_sql_storage_field_storage_query()
.Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedThank for your answer. I'll post problem in Entity Reference issues. But one more interesting thing happen when I set field to not obligatory (like it was before) everything seems to working now. Maybe it is workaround for someone meanwhile.
Comment #37
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is the issue for Entity Reference: #1491454: Alterations of EFQ needs to be made more robust.
Comment #38
salientknight CreditAttribution: salientknight commentedAfter apply the page in #7 non-admin users lost the ability to see views created using Entity References. Changing permissions has not made these views available -- this is a huge issue.
Comment #39
webflo CreditAttribution: webflo commented@salientknight This is a drupal 8 issue. We patch d8 first and backport later.
Comment #40
batje CreditAttribution: batje commentedWe had this issue in D7 and were able to reproduce the issue with the user-references.
The attached patch should not be used!
But it shows the 2 places where we *specifically* check if the entity is a user. If so, we don't invalidate the query.
Basically, the user.module never sets the $entity_info['entity keys']['revision'] and $entity_info['entity keys']['bundle'], but that should not prevent the query to fire.
We are not really sure what to test on. The test for $base_table != 'users' we do now is surely wrong, if only for the reason that the user revision module does add versioning for users and this patch would prevent that to work.
Hope it helps though.
Comment #41
mrsean CreditAttribution: mrsean commentedHi everyone, many thanks to all for the effort that's been put into this!
I too am having this issue when a rule is activated as part of a workflow, where one node references another.
My question is, where does this issue stand now? Is it fixed for D8 but still needs work for D7?
I see that #33 indicates that the D7 version from #23 is the latest correct patch, but doesn't that patch have issues for the admin user?
Just wondering what the status is or what D7 users can do to resolve the isssue, as the D8 patch appears to have passed.
Thanks!
Comment #42
damien_vancouver CreditAttribution: damien_vancouver commented@mrsean,
Here's a summary of where we're at (as I understand it anyway):
- The issue has been verified and we have what looks like a correct patch for D8 and D7, in #23.
- The issue for Entity Reference autocomplete that the #23 was causing was actually a bug in entityreference, #1491454: Alterations of EFQ needs to be made more robust- so the patch in #23 does contain the correct latest version.
Next steps:
- #1491454: Alterations of EFQ needs to be made more robust is marked fixed. We should retest the patch from #23 using the latest available entityreference and verify that the error we were seeing is gone
- In that case patch from #23 seems correct
- I think we need a working simpletest before we can get the patch committed to D8. This involves programatically triggering the bug from a test somehow.
- After patch + test get committed to D8, we can backport them to D7, which should hopefully be straightforward.
- once backported to D7 and tested, that patch can get committed to D7.
So, if you are just looking for a fix: use #23 and report problems here. The fix in #23 will almost certainly be "the fix" once it eventually gets committed. Or it's the latest correct version we have anyway.
If you want to help get this committed, we have two main tasks:
-please specifically test #23 against latest entityreference as above, and report back.
- write the test that reproduces the error, failing without the patch and passing without it.
Comment #43
mshepherd CreditAttribution: mshepherd commentedDamien -
I can only comment on the D7 patch at #23.
With D7.12 with the patch at #23 applied along with Entity Reference 7.x-1.0-rc1 or 7.x-1.0-beta5+1-dev, I can no longer trigger the error as an admin or less privileged user.
Previously I was experiencing this error when using the entity reference pre-populate module, as reported here: https://drupal.org/node/1466404#comment-5832060
I'd mark as RTBC, but I appreciate this thread mostly covers D8.
Thanks!
Matthew
Comment #44
damien_vancouver CreditAttribution: damien_vancouver commentedHi Matthew,
I tested with entiyreference-1.0-rc1, and while I no longer get the error dialog when I create content , I no longer get any results either. No matching users are returned and if I just type in a full username (like "admin") in the autocomplete field and save, the value is lost and the field is not set afterwards. So that is still fully broken I think.
If I roll my entity.inc back to stock Drupal 7.12, then it starts working again and I get results and can autocomplete one of them to set the reference.
Can you test again and see if you get the same results? If so, we should discuss that problem further at #1491454: Alterations of EFQ needs to be made more robust
Before we can RTBC the D8 patch from #23, we need that test. The challenge there is working out how to trigger the error in Drupal 8 without the use of contrib modules. Everyone reporting the bug here is triggering it by having more than one node access module active. But for the Drupal 8 test environment, it will have to work without any of that being available, just with core and what code is written/enabled in the test We either need to implement modules that answer node access hooks for the test suite, or maybe just test the entity query building at a lower level and make sure the prefix is there somehow . If we can figure that out, we can really get this thing moving towards being committed.
Comment #45
vordude CreditAttribution: vordude commented#23 works for me. (so far)
Comment #46
bachbach CreditAttribution: bachbach commented#23 is ok for me with entityreference
Comment #47
Damien Tournoud CreditAttribution: Damien Tournoud commentedJust to clarify: if you are getting the error box using Entity Reference on a user field, it means that the configuration you use for this user field is broken. Most specifically, you are asking Entity Reference to filter by bundles, but the user entity doesn't support that. Please fix the configuration of your field (most probably, you exported it using Features or something like that), simply re-submitting the field configuration page should do it.
The patch in #23 is correct and we should get it in. There is a sufficient test coverage for EFQ to show that it has not broken anything. The issue only occurs when complex access control modules are in effect, so it would be too much a hassle to add a test for this.
Comment #48
mshepherd CreditAttribution: mshepherd commentedHi Damien,
Response to #44.
I get valid results in the field when using entityreference-7.x-1.x-rc1. They get saved properly too. And no SQL error. If there's any more info I can supply, please let me know.
Comment #49
damien_vancouver CreditAttribution: damien_vancouver commentedI can confirm too that everything is working for me. Re-saving the field configuration of my User Reference field as suggested in #47 fixes the issue with the autocomplete, which is just corrupt field configuration from entityreference < 7.x-1.0-beta4.
To fix a broken user reference autocomplete if you still have one:
- update entityreference to 7.x-1.0-rc1 or 7.x-dev.
- run the database update script
- test it again, it might be working.
- If not, then for each non-working user reference field:
1. visit Structure - Content Types - Your Content Type - Manage Fields.
2. Press "Edit" next to the user reference field that isn't working,
3. Hit Save without making any changes on the field settings form. The field's schema will be manually updated with the correct schema and the field should start working.
Meanwhile, #23 is RTBC! Yay!
Comment #50
mrsean CreditAttribution: mrsean commentedThanks for the update Damien.
#23 with the changes to entity reference work for me as well.
Thanks!
Comment #51
mshepherd CreditAttribution: mshepherd commentedExcellent! Good work!
Is there anything anyone here can do to make sure it gets committed?
Matthew
Comment #52
damien_vancouver CreditAttribution: damien_vancouver commentedWe're all finished now that the patch is marked RTBC. We just have to wait and it will get committed to 8.x by one of the maintainers.
So I've set the issue status back to RTBC. Please don't change the status, as that will delay it getting committed.
Then once that 8.x commit happens, we can change the version to 7.x, change the status to "needs review", and re-attach the D7 backport from #23 for testing by the test bot. Assuming that passes (it should) then we can mark it RTBC again against 7.x, and it will get committed to 7.x.
With luck all that will happen quickly enough for the fix to be included in the next Drupal 7 bugfix release.
Comment #53
catchThis still needs an automated test, it was mentioned several times in the issue already, for example in #33 and earlier.
Comment #54
batje CreditAttribution: batje commented(comments are not shown threaded, this is in response to #47
If you are using features and you insist on doing this manually, open the .fields.inc file in your feature and find the field definition. On part of it reads:
'target_bundles' => array('users' => 'users'),
change that to
'target_bundles' => array(),
Make a change on your feature in the gui, and then revert the feature. Done.
Comment #55
dhruba.jyoti.saha CreditAttribution: dhruba.jyoti.saha commented#23 : The patch drupal-1325628-23.patch worked perfectly for me and resolved "PDOException: SQLSTATE[23000]" on entity reference.
Thanks a lot.
Comment #56
dpiThis is not committed.
Comment #57
Tim Jones Toronto CreditAttribution: Tim Jones Toronto commented#23: drupal-1325628-23.patch queued for re-testing.
Comment #58
xjmI'll look into a test.
Comment #59
xjmHere's a start at a test for this. It does not trigger the bug; I am still unclear what the exact requirements to reproduce this bug are at the query level.
For debugging, the test:
Presumably, an entity reference field from the entityreference module is involved in triggering the bug on node update. However, we should be able to emulate whatever EFQ entityreference is running. If someone can provide an example EFQ or more information on what conditions are needed to reproduce, I can finish the test; otherwise, anyone is welcome to try working on it. You can also install the two node access test modules on D8 using drush if you want to experiment with reproducing it through the UI (to come up with steps to reproduce).
Comment #60
xjmComment #61
damien_vancouver CreditAttribution: damien_vancouver commentedThanks for the test code xjm!
I tried to eproduce the error from inside the test, and spent a lot of time with the debugger comparing the failing 7.x code and the 8.x test.
Summary:
It looks like you can't trigger the original error any more in drupal 8, because node_query_entity_field_access_alter uses a subquery, not the join that caused the error in Drupal 7. But I am pretty sure backporting the test to Drupal 7 would allow it to be triggered using a slightly modified EFQ in the test. The error also won't trigger for uid 1, who has 'bypass node access' permissions, as node_access is not joined to in that case. It also looks like it's not caused by multiple node_access modules, and I was able to cause it with just one. Lastly, node.type is also missing a prefix, not just node.nid.
The problem EFQ
I tracked down the EFQ this query begins from, which is in entityreference_field_validate. The query is built in validateReferencableEntities, while it's validating that the referenced entity_id does really exist.
node_access isn't joined to there though, the error is triggered when altered by node_query_entity_field_access_alter(), because it's tagged with 'node_access' by buildEntityFieldQuery().
Bad query with just one node_access module
So - you don't actually need multiple node_access modules to cause the error... even though everyone reporting the error here was using more than one and we all assumed that was the obvious cause. After discovering that, I disabled one of my node_access modules and rebuilt permissions and I can still get the error to happen with just one node_access module. This is the bad query it produces:
'type' is missing a prefix too
I noticed that 'type' is also missing a prefix, and we should also fix that.
ie. WHERE (nid IN (979)) AND (type IN (campaign_theme)) should be WHERE (node.nid IN (979)) AND (node.type IN (campaign_theme)).
In 8.x this appears to be added by entity.query.inc line 809 and looks like it should have a prefix. 7.x looks similar (includes/entity.inc line 1196). Needs further investigation and the fix added to #23.
Why the "Column 'nid' in where clause is ambiguous" error can't be triggered any more in Drupal 8:
In Drupal 8, the implementation of node_query_entity_field_access_alter() has changed to use a Subquery, not a join.
7.x modules/node/node.module line 3253
8.x: core/modules/node/node.module line 3249
It's that join to node_access in the 7.x that causes column 'nid' to be ambiguous and triggers the error.
Triggering the error in Drupal 7 using the test case
We should be able to trigger it in D7 though with the following, which should be identical to the EFQ that entityreference is running:
it also needs to run as a user without 'bypass node access' permission, as that skips the node_access join.
Backport vs 8.x
For 8.x, since we can't trigger it, we will just have to check and see if the entityCondition for entity_type and entity_id is properly prefixing somehow.
While we don't need to test multiple node access modules, I think that multiple node_access module test is really great coverage and should stick around in 8.x. But for the 7.x backport we could/should remove it as it's not needed to trigger the error.
While the two tests could diverge a lot (ie. it'd be nice to trigger the original error in the 7.x test), the bug of EFQ propertyQuery not adding the prefix for node.type or node.nid remains the same in both versions.
Required additions to #23 fix
#23 needs modifying to properly prefix node.type as well, or we should figure out why entity.query.inc line 809:
$select_query->addExpression(':entity_type', 'entity_type', array(':entity_type' => $entity_type));
is not properly prefixing the type with 'entity_type' (parameter 2). That could be a different bug somewhere other than EFQ propertyQuery(). I haven't yet figured out how to verify this is still happening in 8.x, but it definitely is in 7.x as shown by my failing query.
Comment #62
xjmWow, excellent writeup!
It sounds to me like the thing to do is to add tests to
EntityFieldQueryTestCase
to assert that thetest casetable name is prefixed. A second test for the D7 functional bug(s) here could be helpful, but it's also a bit of an edge case. In either case, we'd want the same tests in both branches to the extent that the expected functionality is the same in both branches.(Edited.)
Comment #63
xjmSee also: #1302228: field_has_data() returns inconsistent results – possible data loss
Comment #64
damien_vancouver CreditAttribution: damien_vancouver commentedOK, I'll work on some new tests as xjm suggested in #62, and also investigate that missing "type" prefix.
Comment #65
damien_vancouver CreditAttribution: damien_vancouver commentedHere's an update on my work towards a test for this issue:
Unfortunately though, the select query is built entirely inside the internals of EFQ - it is created inside ->execute() and then passed to ->finishQuery. The results of the query are returned in an array, but never the query object itself. So, I don't think it's possible to test for the prefixes being there in EntityFieldQueryTest.
Also, EntityFieldQueryTest is inside the contrib Entity API module in D7, so we'd have to create a working similar test in node.test or something.
I'm still working on a new test using hook_query_alter to cause an exception.
Comment #66
damien_vancouver CreditAttribution: damien_vancouver commentedI have a working test!
[EDIT: Use the test from #67 instead of this one, it shows the actual PDO error]
I've added a NodeAccessEFQTablePrefixTestCase to the end of the Node tests, which runs this simple node EFQ:
Then the query is altered (a la the old node_access alteration) by a helper module:
This causes node to join to itself, and so any non-prefixed columns will be ambiguous and cause a PDOException.
Attaching test only for D8, hopefully the test bot sees it!
Comment #67
damien_vancouver CreditAttribution: damien_vancouver commentedHere is an improved version of the test from #66 - this one logs the actual PDO Exception in the test output.
Comment #69
Berdir@damien_vancouver: Remember to always upload a test-only patch (to prove that the test fails without the fix) and second the patch with test and the fix, to show that it works with the fix.
Comment #70
damien_vancouver CreditAttribution: damien_vancouver commented@Berdir, I guess I could have attached them to the same comment.. is that the preferred procedure? I was waiting to see it fail first.
Anyway, here is the full patch: the failing test from #67 with the patch from #23 applied.
Comment #71
BerdirYes, exactly. First the test-only patch, to see that it fails and second the complete patch with tests and fix. That way, the testbot will mark the first patch as red, the second as green and will keep the issue at needs review
Comment #72
damien_vancouver CreditAttribution: damien_vancouver commentedOK, I'll attempt attaching both to this comment.
I tidied everything up so that it will hopefully be RTBC if it passes.
Changes:
- renamed the test class to no longer mention NodeAccess, which is no longer part of the test (I self join to node instead)
- removed some old verbose debugging code that printing out the entire query object
- tidied up the name and description of the test so that capitalization etc. was consistent with the other node tests.
I'm re-attaching test only followed by complete patch. This could be the one!
Comment #73
mshepherd CreditAttribution: mshepherd commentedgreat work damien! fingers crossed!
Comment #74
damien_vancouver CreditAttribution: damien_vancouver commentedThe test is working - It looks like this just needs some 3rd party review and is RTBC. I can make a 7.x backport afterwards.
I just tested and can confirm that in Drupal 7.14 the error no longer happens due to node_access (thanks to #681760: Try to improve performance and eliminate duplicates caused by node_access table joins).
So if you applied #23 or an earlier version of this patch, you should be able to just upgrade to 7.14 and the error will be gone.
Only queries altered with a join to node, node_access, or other tables with a 'nid' column will cause the error now. The common way to trigger the error should be gone.
Comment #75
mshepherd CreditAttribution: mshepherd commentedDamien,
Just to clarify, is it the test or the patch that now needs review?
Matthew
Comment #76
damien_vancouver CreditAttribution: damien_vancouver commentedThe patch to be tested is the complete patch from #72 - drupal_node_efq_table_prefix-1325628-72.patch. It is against 8.x-dev. That patch was created from my test, plus applying #23 (which is the most widely tested on this thread so far).
Once that is reviewed, RTBC, and committed, we can make a 7.x backport which will need testing against Drupal 7 before being RTBC and committed there.
So if you don't have Drupal 8, you could help with the testing of the backport when that's ready.
It should be much harder to trigger this error in Drupal 7.14, because of other bugfixes. The bug is there still, but node_access queries don't set it off any more. I upgraded two problem sites to 7.14 and both of them are working OK without the patch for me. If anyone is still seeing the error after 7.14, please post here and we'll get the backport ready right away.
Comment #77
mshepherd CreditAttribution: mshepherd commentedI just migrated a site I was having a problem with into D7.14 and confirmed that I can no longer trigger the error.
Comment #78
Tim Jones Toronto CreditAttribution: Tim Jones Toronto commentedI have tested using latest Drupal 7.14 + Domain Access 7.x-3.x-dev + Entity reference.
I am still getting the known error:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'nid' in where clause is ambiguous: …. ETC…
As soon as a version 7 backport is available, I can test it immediately and report back here.
Comment #79
damien_vancouver CreditAttribution: damien_vancouver commentedThe Drupal 7.14 fix is attached as a backported patch file, and also a renamed includes/entity.inc.
Fixing by replacing entity.inc (easy)
save the attached entity.inc-patched-drupal7.14-1325628-79.txt and then rename it to entity.inc.
Copy the renamed entity.inc over top of your old Drupal 7.14 includes/entity.inc and the error will go away.
Fixing by testing the patch (awesome)
Apply drupal_node_efq_table_prefix_7.x-1325628-79-do-not-test.patch.
Try it out and report any problems you have here! We are pretty much certain this is the problem and a working fix, so there is no need to post that it's working for you... but do post any problems.
Testing the test: If you revert entity.inc back to the stock Drupal 7.14 one, then the node_efq_table_prefix_test will fail with a PDOException. The complete patch should pass. We will verify this using the Drupal testbot when the issue is set to 7.x (after we get the 8.x one committed).
Please do not touch the status or version of the issue, as we are working on getting the fix into 8.x first and that will just slow things up.
Comment #80
Tim Jones Toronto CreditAttribution: Tim Jones Toronto commentedHi Damien, thanks.
I have:
1. Replaced SITE/includes/entity.inc with entity.inc-patched-drupal7.14-1325628-79.txt link and tested with Drupal 7.14.
2. Retested my site - node editing and updates and confirmed to be working without errors. Cool!
Happy to test the patch 'drupal_node_efq_table_prefix_7.x-1325628-79-do-not-test.patch' and will report back here.
Comment #81
Tim Jones Toronto CreditAttribution: Tim Jones Toronto commentedUsing #79 drupal_node_efq_table_prefix_7.x-1325628-79-do-not-test.patch, I have patched:
RESULTS:
1. On node editing of an entity on my site, everything worked fine with no error.
2. Ran node entity field query table prefixes test (wasn't sure if you wanted?):
Will continue to test this patch with the site and confirm stability with content design/use. So far looking good!
Comment #82
John Pitcairn CreditAttribution: John Pitcairn commentedI've managed to trigger this error in 7.14. Applying the patch fixes it, thanks.
Error:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'nid' in where clause is ambiguous: SELECT DISTINCT node.nid AS entity_id, node.vid AS revision_id, node.type AS bundle, :entity_type AS entity_type FROM {node} node INNER JOIN {node_access} na ON na.nid = node.nid WHERE (nid IN (:db_condition_placeholder_0)) AND (type IN (:db_condition_placeholder_1)) AND(( (na.gid = :db_condition_placeholder_2) AND (na.realm = :db_condition_placeholder_3) )OR( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) )OR( (na.gid = :db_condition_placeholder_8) AND (na.realm = :db_condition_placeholder_9) ))AND (na.grant_view >= :db_condition_placeholder_10) ORDER BY node.title ASC; Array ( [:db_condition_placeholder_0] => 19 [:db_condition_placeholder_1] => programme [:db_condition_placeholder_2] => 0 [:db_condition_placeholder_3] => all [:db_condition_placeholder_4] => 0 [:db_condition_placeholder_5] => domain_site [:db_condition_placeholder_6] => 3 [:db_condition_placeholder_7] => domain_id [:db_condition_placeholder_8] => 1 [:db_condition_placeholder_9] => view_unpublished_content [:db_condition_placeholder_10] => 1 [:entity_type] => node ) in EntityFieldQuery->execute() (line 1139 of /Volumes/Opus Locus/Sites/_drupal7/core/includes/entity.inc).
Comment #83
catchtagging.
Comment #84
damien_vancouver CreditAttribution: damien_vancouver commentedHi catch,
There's actually a full 7.x backport attached to #79, with test. It hasn't run through the test bot yet but it should be ready to go: drupal_node_efq_table_prefix_7.x-1325628-79-do-not-test.patch.
Then for 8.x, the patches on #72 should be RTBC, as long as they still apply cleanly. If not I will re-roll.
Comment #85
damien_vancouver CreditAttribution: damien_vancouver commented#72: drupal_node_efq_table_prefix-1325628-72.patch queued for re-testing.
Comment #87
xjm@damien_vancouver, the "Needs backport to D7" tag just means that this issue should be moved to the 7.x branch once it is committed to 8.x--not that it's "missing" a D7 patch.
Comment #88
xjmCleaning this up now.
Comment #89
xjmAttached:
No interdiff is included since most lines were either moved to different files or changed entirely.
Comment #90
Damien Tournoud CreditAttribution: Damien Tournoud commentedHow many times do I need to say that the code with
1 = 0
is what we want?EntityFieldQuery::propertyQuery()
andfield_sql_storage_field_storage_query()
should have the same code for handling bundles. They currently do not.Comment #91
xjmI have no idea what this means. There is a working patch in the issue, with test coverage. Can you clarify your review?
Edit: just found this up in #3:
That seems like a terrible hack to me. No, that is not what we want?
Comment #93
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt is (1) not a hack, (2) the expected behavior if there is no bundle on an entity: if you try to filter per bundle you are going to end-up with nothing, (3) just a copy of the code that *already* exists in
field_sql_storage_field_storage_query()
that we failed to keep on par.The current patch has this, which is completely bogus:
Comment #94
xjmSo, is the attached what you have in mind? (Just want to move this issue forward.)
Comment #97
xjmWithout the copypaste fail.
Comment #99
xjmNot reverting the fix this time. The test-only patch is the same.
Comment #100
chx CreditAttribution: chx commentedThere is a (lot of) confusion oozing from this issue due to the missing definition of entities. However, as we are in a code thaw, we are allowed to change that.
An entity is an object with some metadata attached to it (in hook_entity_info). There's an unspoken assumption that they are, by default, stored in an SQL table with their properties mapped into a column name 1:1. We make this assumption when we take properties passed to propertyCondition and pass them to SQL. Despite the fact that MySQL for example can not have longer than 64 character column names while PHP identifiers are only memory bound. Of course, this assumption also falls apart when the entity contains computed columns like bundles for comments. Let's not talk of the uid in a
$node
...There is no "correct" or "incorrect" solution to this problem. We need to clarify what it is we want, what it is we expect from entities and then patch.
Comment #101
xjmSo I'm not particularly keen on the
WHERE 1 = 0
, and I'm not sure why the previous fallback condition was "completely bogus" (details would help), but as this is a functional bug chx suggested opening a followup for the situation where the bundle is not defined. I don't particularly understand what would be in the scope of that issue so I have not filed it. Is #99 okay?Comment #102
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgain, this
1 = 0
condition is not new. We are just bringing the two query generation paths of EFQ (the property-only path and the field path) in par one another. The discussion about the behavior is correct or not doesn't belong to this issue.#99 looks correct to me.
Comment #103
chx CreditAttribution: chx commentedOK, but let's open a followup for the discussion in #100.
Comment #106
xjmThanks @Damien Tournoud!
Trying to file this followup issue... I looked in field_sql_storage_field_storage_query() and couldn't figure out what maps to the
1 = 0
condition in that function. Can you help me locate the relevant code for this so we can create that followup?Comment #107
xjm7.x backport attached.
I also tested the 7.x tests and complete patch in #1635240: Disregard -- patch testing issue ☢.
Comment #108
jienckebd CreditAttribution: jienckebd commented#79 worked for me -- thanks Damien!
Comment #109
purserj CreditAttribution: purserj commentedHi all, just wondering if this patch is going to be committed to 7 core?
Comment #110
xjm@purserj: The issue is tagged "needs backport to D7" and there is a D7 patch available in #107.
Comment #111
Dries CreditAttribution: Dries commentedFinally had a chance to review this and committed it to 8.x. Moving the version to 7.x.
Comment #112
xjmLet's reupload the D7 test-only and combined patches for the bot. :)
Comment #113
xjmHere we go. Should still be RTBC.
Comment #114
Anonymous (not verified) CreditAttribution: Anonymous commented#113: efq_prefix-132562-100-d7-tests.patch queued for re-testing.
Comment #115
Anonymous (not verified) CreditAttribution: Anonymous commentedI tried the combined patch from #113 and it solved my ambiguous issue!
Comment #116
David_Rothstein CreditAttribution: David_Rothstein commentedI think this makes sense, but just want to reupload to make sure the tests still pass. I also fixed this:
which is definitely not correct for Drupal 7 :)
Comment #117
David_Rothstein CreditAttribution: David_Rothstein commentedOK, it all seems to check out. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/55fbef3
Comment #119
xjm