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 {
CommentFileSizeAuthor
#116 efq-prefix-1325628-116-TESTS-ONLY.patch1.78 KBDavid_Rothstein
#116 efq-prefix-1325628-116.patch4.39 KBDavid_Rothstein
#113 efq_prefix-132562-100-d7-tests.patch1.8 KBxjm
#113 efq_prefix-132562-100-d7-combined.patch4.4 KBxjm
#107 efq_prefix-132562-100-d7-do_not_test.patch4.4 KBxjm
#99 efq_prefix-1325628-99-combined.patch4.73 KBxjm
#99 interdiff-89-99.txt705 bytesxjm
#97 efq_prefix-1325628-95-tests.patch1.98 KBxjm
#97 efq_prefix-1325628-95-combined.patch4.71 KBxjm
#94 efq_prefix-1325628-94-tests.patch1.98 KBxjm
#94 efq_prefix-1325628-94-combined.patch4.72 KBxjm
#94 interdiff.txt1.01 KBxjm
#89 efq_prefix-1325628-89-tests.patch1.98 KBxjm
#89 efq_prefix-1325628-89-complete.patch4.73 KBxjm
#79 drupal_node_efq_table_prefix_7.x-1325628-79-do-not-test.patch4.5 KBdamien_vancouver
#79 entity.inc-patched-drupal7.14-1325628-79.txt44.89 KBdamien_vancouver
#72 drupal_node_efq_table_prefix_test_only-1325628-72.patch3.14 KBdamien_vancouver
#72 drupal_node_efq_table_prefix-1325628-72.patch4.9 KBdamien_vancouver
#70 drupal_node_efq_table_prefix-1325628-70.patch4.89 KBdamien_vancouver
#66 drupal_node_efq_table_prefix_test_only-1325628-66.patch3.1 KBdamien_vancouver
#67 drupal_node_efq_table_prefix_test_only-1325628-67.patch3.13 KBdamien_vancouver
#59 efq_node_access_test.patch4.87 KBxjm
#33 drupal-entity_fully_qualified_db_column_names_drupal7.12-1325628-33.patch2.6 KBdamien_vancouver
#33 drupal-entity_fully_qualified_db_column_names_d8-1325628-33.patch2.66 KBdamien_vancouver
#33 drupal-entity_comment31_error-1325628-33.png27.4 KBdamien_vancouver
#32 error.png10.4 KBAnonymous (not verified)
#27 Create Userref localhost.jpg56.8 KBwipeout_dude
#23 drupal-1325628-23.patch2.16 KBTravisCarden
#23 drupal-1325628-23-d7.patch2.03 KBTravisCarden
#22 drupal_entity-fully_qualified_db_column_names_D7.12-1325628-22.patch1.76 KBdamien_vancouver
#22 drupal_entity_patched_entity.inc_drupal_7.12-1325628-22.txt44.86 KBdamien_vancouver
#21 2012-01-23_13-37-04.jpg37.32 KBwipeout_dude
#11 drupal_entity-fully_qualified_db_column_names-1325628-11.patch1.69 KBdamien_vancouver
#7 drupal_entity-fully_qualified_db_column_names_drupal7_backport-1325628-7.patch1.62 KBdamien_vancouver
#6 drupal_entity-fully_qualified_db_column_names_drupal7_backport-1325628-6.patch1.65 KBdamien_vancouver
#3 1325628-entity-field-query-property-qualified.patch1.64 KBDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Version: 7.9 » 8.x-dev

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

xjm’s picture

Status: Needs review » Active
Issue tags: +Novice

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

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
1.64 KB

At 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 with EntityFieldQuery::propertyQuery() with regard to support for entities without a bundle... we need to check this
  • We need to confirm if it is standard SQL to ORDER BY on a column alias.

Status: Needs review » Needs work

The last submitted patch, 1325628-entity-field-query-property-qualified.patch, failed testing.

xjm’s picture

Issue tags: -Novice

Untagging.

damien_vancouver’s picture

I 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 ]

damien_vancouver’s picture

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

pjcdawkins’s picture

The patch in #7 works for me in Drupal 7.10.

[edit: and the same patch also works in Drupal 7.11]

dddbbb’s picture

#7 works for me too (Drupal 7.8). Many thanks.

clashar’s picture

#7 worked for me as well (D 7.8)

damien_vancouver’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

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

  // Old patch contained line:
    $this->addCondition($select_query, $sql_field, $this->entityConditions['bundle'], $having);
  // But that line should also have the $base_table prefix.
    $this->addCondition($select_query, "$base_table". $sql_field, $this->entityConditions['bundle'], $having);

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:

2. We need to confirm if it is standard SQL to ORDER BY on a column alias.

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 :

4) If ORDER BY is specified, then each <sort specification> in the
<order by clause> shall identify a column of T.

Case:

a) If a <sort specification> contains a <column name>, then T
shall contain exactly one column with that <column name> and
the <sort specification> identifies that column.

Then the definition of <order by clause> also points back to a <column name>

<order by clause> ::=
ORDER BY <sort specification list>

<sort specification list> ::=
<sort specification> [ { <comma> <sort specification> }... ]

<sort specification> ::=
<sort key> [ <collate clause > ] [ <ordering specification> ]

<sort key> ::=
<column name>
| <unsigned integer>

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:

1) A <correlation name> immediately contained in a <table refer-
ence> TR is exposed by TR. A <table name> immediately contained
in a <table reference> TR is exposed by TR if and only if TR
does not specify a <correlation name>.

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

Status: Needs review » Needs work

The last submitted patch, drupal_entity-fully_qualified_db_column_names-1325628-11.patch, failed testing.

wipeout_dude’s picture

#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??

wipeout_dude’s picture

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

BTMash’s picture

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

damien_vancouver’s picture

@BTMash,

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.

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!

BTMash’s picture

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

damien_vancouver’s picture

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

BTMash’s picture

I 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

class SampleEntityAccessTestCase extends DrupalWebTestCase {
  function setUp() {
    parent::setUp('testing_entity_access_module');
    ...
  }
  ...
}

That will ensure that testing_entity_access_module gets enabled/installed for the particular test.

damien_vancouver’s picture

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

wipeout_dude’s picture

FileSize
37.32 KB

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

damien_vancouver’s picture

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

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
2.16 KB

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

squaretone’s picture

TravisCarden's patch (drupal-1325628-23-d7.patch) fixed the issue for me.

damien_vancouver’s picture

Hey 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?

damien_vancouver’s picture

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

        $select_query->orderBy("$base_table." . $order['specifier'], $order['direction']);

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?

wipeout_dude’s picture

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

damien_vancouver’s picture

Status: Needs review » Needs work

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

      $conditions = &$query->conditions();
      foreach ($conditions as $key => $condition) {
        if ($condition['field'] == 'users.name') {

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:

    if (isset($this->entityConditions['bundle'])) {
      if (!empty($entity_info['entity keys']['bundle'])) {
        $this->addCondition($select_query, "$base_table." . $sql_field, $this->entityConditions['bundle'], $having);
      }
      else {
        // This entity has no bundle, invalidate the query.
        $select_query->where('1 = 0');
      }      
    }

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:

@Damien Tournoud: field_sql_storage_field_storage_query() doesn't seem to be on par with EntityFieldQuery::propertyQuery() with regard to support for entities without a bundle... we need to check this

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.

webmaster-eddie’s picture

Version: 8.x-dev » 7.12

Just 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

TravisCarden’s picture

Version: 7.12 » 8.x-dev

Thanks 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! :)

Xomby’s picture

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

Anonymous’s picture

FileSize
10.4 KB

Hi,
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?

damien_vancouver’s picture

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

  • User reference works as before now with no ajax error (see #27 for steps to test/reproduce)
  • The original issue is also fixed by the patch, as expected.

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.

Anonymous’s picture

Status: Needs work » Needs review

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

Damien Tournoud’s picture

Status: Needs review » Needs work

The 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 in EntityFieldQuery::propertyQuery() and applies it to field_sql_storage_field_storage_query().

Anonymous’s picture

Status: Needs review » Needs work

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

Damien Tournoud’s picture

Here is the issue for Entity Reference: #1491454: Alterations of EFQ needs to be made more robust.

salientknight’s picture

Version: 8.x-dev » 7.9
Priority: Normal » Major

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

webflo’s picture

Version: 7.9 » 8.x-dev

@salientknight This is a drupal 8 issue. We patch d8 first and backport later.

batje’s picture

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

mrsean’s picture

Hi 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!

damien_vancouver’s picture

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

mshepherd’s picture

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

damien_vancouver’s picture

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

vordude’s picture

#23 works for me. (so far)

bachbach’s picture

#23 is ok for me with entityreference

Damien Tournoud’s picture

Status: Needs work » Reviewed & tested by the community

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

mshepherd’s picture

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

damien_vancouver’s picture

I 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!

mrsean’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the update Damien.

#23 with the changes to entity reference work for me as well.

Thanks!

mshepherd’s picture

Excellent! Good work!

Is there anything anyone here can do to make sure it gets committed?

Matthew

damien_vancouver’s picture

Status: Needs work » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This still needs an automated test, it was mentioned several times in the issue already, for example in #33 and earlier.

batje’s picture

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

dhruba.jyoti.saha’s picture

Version: 8.x-dev » 7.0-rc1
Status: Needs work » Fixed

#23 : The patch drupal-1325628-23.patch worked perfectly for me and resolved "PDOException: SQLSTATE[23000]" on entity reference.

Thanks a lot.

dpi’s picture

Version: 7.0-rc1 » 8.x-dev
Status: Fixed » Needs work

This is not committed.

Tim Jones Toronto’s picture

Status: Needs work » Needs review

#23: drupal-1325628-23.patch queued for re-testing.

xjm’s picture

Assigned: Unassigned » xjm

I'll look into a test.

xjm’s picture

FileSize
4.87 KB

Here'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:

  • Enables both the existing D8 node access test module and an additional one I've added.
  • Creates and authenticates a user with permissions from both modules.
  • Creates an article node.
  • Tests an EFQ with entity conditions for the entity type and bundle.
  • Tests viewing the node.
  • Tests editing the node.

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

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs review » Needs work
damien_vancouver’s picture

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

  $entity_type = $this->field['settings']['target_type'];
  $query = $this->buildEntityFieldQuery();  // among other things, this sets the 'node_access' tag.
  $query->entityCondition('entity_id', $ids, 'IN');

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:

SELECT DISTINCT node.nid AS entity_id, node.vid AS revision_id, node.type AS bundle, 'node' AS entity_type 
FROM {node} node INNER JOIN {node_access} na ON na.nid = node.nid 
WHERE 
(nid IN (979)) AND (type IN (campaign_theme))  /* Note: both these need prefixing, not just nid! */
AND
(
  ( (na.gid = 0) AND (na.realm = 'all') )
  OR
  ( (na.gid = 4) AND (na.realm = 'group_access_authenticated') )
  OR
  ( (na.gid = 11) AND (na.realm = 'group_access_authenticated') )
)
AND (na.grant_view >= 1) 
ORDER BY node.title ASC;
'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.

  $select_query->addExpression(':entity_type', 'entity_type', array(':entity_type' => $entity_type));  // why is 'entity_type' not the prefix then?
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

   if (!($table instanceof SelectQueryInterface) && $table == $base_table) {

      // The node_access table has the access grants for any given node so JOIN
      // it to the table containing the nid which can be either the node
      // table or a field value table.
      if ($type == 'node') {
        $access_alias = $query->join('node_access', 'na', '%alias.nid = ' . $nalias . '.nid');

8.x: core/modules/node/node.module line 3249

  if (!($table instanceof SelectInterface) && $table == $base_table) {
      // Set the subquery.
      $subquery = db_select('node_access', 'na')
       ->fields('na', array('nid'));

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:

 // Run an EFQ with multiple EntityConditions.
    // @todo This does not trigger the bug.
    $query = new EntityFieldQuery();
    $query
      ->entityCondition('entity_type', 'node')
      ->entityCondition('bundle', 'article')
      ->entityCondition('entity_id', array($this->node->nid), 'IN')
      ->addTag('node_access')
      ->propertyCondition('status', 1);

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.

xjm’s picture

Wow, excellent writeup!

It sounds to me like the thing to do is to add tests to EntityFieldQueryTestCase to assert that the test case table 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.)

xjm’s picture

damien_vancouver’s picture

Assigned: Unassigned » damien_vancouver

OK, I'll work on some new tests as xjm suggested in #62, and also investigate that missing "type" prefix.

damien_vancouver’s picture

Here's an update on my work towards a test for this issue:

  • That missing prefix on "type" I was seeing was just because I was unpatched, #23 is fine.
  • It's no longer possible to trigger the error in the latest 7.x code either, because of #681760: Try to improve performance and eliminate duplicates caused by node_access table joins - the same thing preventing it from occurring in D8. So most likely everyone seeing this error (as a result of node access problems) will no longer see it in the next 7.x bugfix release. This is good news!
  • The problem remains thoguh, and anything doing a hook_query_alter and joining to node would probably cause the error again.
  • I looked into creating a test as suggested by xjm in EntityFieldQueryTest.

    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.

damien_vancouver’s picture

I 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:

$query
      ->entityCondition('entity_type', 'node')
      ->entityCondition('bundle', 'article')
      ->entityCondition('entity_id', $this->node->nid, '=')
      ->addTag('node_efq_table_prefix_test');

Then the query is altered (a la the old node_access alteration) by a helper module:

function node_efq_table_prefix_test_query_alter($query) {
  if ($query->hasTag('node_efq_table_prefix_test')) {
    $query->join('node','n2','%alias.nid = node.nid');
  }
}

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!

damien_vancouver’s picture

Assigned: damien_vancouver » Unassigned
Status: Needs work » Needs review
FileSize
3.13 KB

Here is an improved version of the test from #66 - this one logs the actual PDO Exception in the test output.

Status: Needs review » Needs work

The last submitted patch, drupal_node_efq_table_prefix_test_only-1325628-67.patch, failed testing.

Berdir’s picture

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

damien_vancouver’s picture

Status: Needs work » Needs review
FileSize
4.89 KB

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

Berdir’s picture

Yes, 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

damien_vancouver’s picture

OK, 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!

mshepherd’s picture

great work damien! fingers crossed!

damien_vancouver’s picture

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

mshepherd’s picture

Damien,
Just to clarify, is it the test or the patch that now needs review?
Matthew

damien_vancouver’s picture

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

mshepherd’s picture

I just migrated a site I was having a problem with into D7.14 and confirmed that I can no longer trigger the error.

Tim Jones Toronto’s picture

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

damien_vancouver’s picture

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

Tim Jones Toronto’s picture

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

Tim Jones Toronto’s picture

Using #79 drupal_node_efq_table_prefix_7.x-1325628-79-do-not-test.patch, I have patched:

  • includes/entity.inc
  • modules/node/node.test
  • modules/node/tests/node_efq_table_prefix_test.info
  • modules/node/tests/node_efq_table_prefix_test.module

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?):

2 passes, 0 fails, and 0 exceptions with results as:

----------------------------------------------------------------------------------------------------------------------------
Enabled modules: node_efq_table_prefix_test - Other - node.test	- line 2511 - NodeEFQTablePrefixTestCase->setUp() **PASS
----------------------------------------------------------------------------------------------------------------------------
Node EntityFieldQuery properly prefixes SQL column names with SQL table names - Other - node.test line 2541 NodeEFQTablePrefixTestCase->testNodeEFQTablePrefixes() **PASS
----------------------------------------------------------------------------------------------------------------------------

Will continue to test this patch with the site and confirm stability with content design/use. So far looking good!

John Pitcairn’s picture

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

catch’s picture

Issue tags: +Needs backport to D7

tagging.

damien_vancouver’s picture

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

damien_vancouver’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The last submitted patch, drupal_node_efq_table_prefix-1325628-72.patch, failed testing.

xjm’s picture

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

xjm’s picture

Assigned: Unassigned » xjm

Cleaning this up now.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.73 KB
1.98 KB

Attached:

  • Updates the patch for PSR-0
  • Moves the tests into entity.module, as the bug is not actually specific to the node access system.
  • Cleans up some documentation and code style.
  • Simplifies the test code.

No interdiff is included since most lines were either moved to different files or changed entirely.

Damien Tournoud’s picture

Status: Needs review » Needs work

How many times do I need to say that the code with 1 = 0 is what we want?

EntityFieldQuery::propertyQuery() and field_sql_storage_field_storage_query() should have the same code for handling bundles. They currently do not.

xjm’s picture

How many times do I need to say that the code with 1 = 0 is what we want?

I 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:

+++ b/core/includes/entity.incundefined
@@ -1237,7 +1237,13 @@ class EntityFieldQuery {
-      $this->addCondition($select_query, $sql_field, $this->entityConditions['bundle'], $having);
+      if (!empty($entity_info['entity keys']['bundle'])) {
+        $this->addCondition($select_query, $sql_field, $this->entityConditions['bundle'], $having);
+      }
+      else {
+        // This entity has no bundle, invalidate the query.
+        $select_query->where('1 = 0');
+      }

That seems like a terrible hack to me. No, that is not what we want?

Damien Tournoud’s picture

That seems like a terrible hack to me. No, that is not what we want?

It 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:

+      if (!empty($entity_info['entity keys']['bundle'])) {
+        $this->addCondition($select_query, $base_table . '.' . $sql_field, $this->entityConditions['bundle'], $having);
+      }
+      else {
+        $this->addCondition($select_query, $sql_field, $this->entityConditions['bundle'], $having);
+      }
xjm’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
4.72 KB
1.98 KB

So, is the attached what you have in mind? (Just want to move this issue forward.)

Status: Needs review » Needs work

The last submitted patch, efq_prefix-1325628-94-combined.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.71 KB
1.98 KB

Without the copypaste fail.

Status: Needs review » Needs work

The last submitted patch, efq_prefix-1325628-95-combined.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
705 bytes
4.73 KB

Not reverting the fix this time. The test-only patch is the same.

chx’s picture

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

xjm’s picture

So 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?

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

OK, but let's open a followup for the discussion in #100.

xjm’s picture

Thanks @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?

xjm’s picture

7.x backport attached.

I also tested the 7.x tests and complete patch in #1635240: Disregard -- patch testing issue ☢.

jienckebd’s picture

#79 worked for me -- thanks Damien!

purserj’s picture

Hi all, just wondering if this patch is going to be committed to 7 core?

xjm’s picture

@purserj: The issue is tagged "needs backport to D7" and there is a D7 patch available in #107.

Dries’s picture

Version: 8.x-dev » 7.x-dev

Finally had a chance to review this and committed it to 8.x. Moving the version to 7.x.

xjm’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Let's reupload the D7 test-only and combined patches for the bot. :)

xjm’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
4.4 KB
1.8 KB

Here we go. Should still be RTBC.

Anonymous’s picture

Anonymous’s picture

I tried the combined patch from #113 and it solved my ambiguous issue!

David_Rothstein’s picture

I think this makes sense, but just want to reupload to make sure the tests still pass. I also fixed this:

+ * @see Drupal\entity\Tests\EntityFieldQueryTest::testTablePrefixing()

which is definitely not correct for Drupal 7 :)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.15 release notes

OK, it all seems to check out. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/55fbef3

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned