hi,

tried using domain module, but getting this error in menu_tree_check_access():

Error message
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'nid' in where clause is ambiguous: SELECT DISTINCT node.nid AS nid FROM {node} node INNER JOIN {domain_access} da_admin ON da_admin.nid = node.nid WHERE (status = :db_condition_placeholder_0) AND (nid IN (:db_condition_placeholder_1)) AND(( (da_admin.gid = :db_condition_placeholder_2) AND (da_admin.realm = :db_condition_placeholder_3) )OR( (da_admin.gid = :db_condition_placeholder_4) AND (da_admin.realm = :db_condition_placeholder_5) )); Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => 138 [:db_condition_placeholder_2] => 0 [:db_condition_placeholder_3] => domain_site [:db_condition_placeholder_4] => 0 [:db_condition_placeholder_5] => domain_id ) in menu_tree_check_access() (line 1285 of /drupal7/includes/menu.inc).

changing
$select->condition('nid', $nids, 'IN');
to
$select->condition('node.nid', $nids, 'IN');

worked for me. mini patch attached, please review

regards

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pebosi’s picture

Status: Active » Needs review
gdd’s picture

Status: Needs review » Reviewed & tested by the community

This can happen with node_access queries that bring in other tables that also have the column 'nid'. This patch works and still applies, and it is the right solution.

Dries’s picture

Is it the right solution? What if we have database prefixing enabled? Shouldn't we create an alias for the node table, say 'n', and write 'n.nid'?

pebosi’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
818 bytes
785 bytes

Ok, i did forget prefixing...created two patches with two different possibilities, please review again

regards

Dries’s picture

I like 766382_2.patch best but let's see. :)

pebosi’s picture

Ok the first patch was dumb :)

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

hmmm. 0 passes with 766382_2. i'll try a re-test. code looks good.

moshe weitzman’s picture

#4: 766382_2.patch queued for re-testing.

pebosi’s picture

#4: 766382_2.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

Suppose 'status' condition should be prefixed too.

andypost’s picture

Status: Fixed » Needs review
FileSize
683 bytes

Resulting query

SELECT n.nid AS nid
FROM 
{node} n
WHERE  (status = :db_condition_placeholder_0) AND (n.nid IN  (:db_condition_placeholder_1)) 

So status should be prefixed too

Dries’s picture

Status: Needs review » Fixed

You're right, Andy. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

mvaneyken’s picture

Problem appears to have resurfaced with Drupal core 7.2.

Situation:
Am able to list & click on book title as a registered user (site administrator). As anonymous user, I can list, but not click on the book titles. Clicking on the book title results in:
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'node.nid' in 'where clause': SELECT DISTINCT n.title AS title FROM {node} n INNER JOIN {node_access} na ON na.nid = n.nid WHERE (node.nid = :db_condition_placeholder_0) AND (n.language IN (:db_condition_placeholder_1, :db_condition_placeholder_2)) AND(( (na.gid = :db_condition_placeholder_3) AND (na.realm = :db_condition_placeholder_4) )OR( (na.gid = :db_condition_placeholder_5) AND (na.realm = :db_condition_placeholder_6) ))AND (na.grant_view >= :db_condition_placeholder_7) ; Array ( [:db_condition_placeholder_0] => 33 [:db_condition_placeholder_1] => en [:db_condition_placeholder_2] => und [:db_condition_placeholder_3] => 0 [:db_condition_placeholder_4] => all [:db_condition_placeholder_5] => 0 [:db_condition_placeholder_6] => abt [:db_condition_placeholder_7] => 1 ) in book_block_view() (line 292 of /home/consult/public_html/modules/book/book.module).

Fix:
Changed book.module, function book_block_view($delta = ''), condition('nid') to condition('n.nid')
IE:
...
elseif ($current_bid) {
// Only display this block when the user is browsing a book.
// MVE test patch:
// $select = db_select('node', 'n')
// ->fields('n', array('title'))
// ->condition('nid', $node->book['bid'])
// ->addTag('node_access');
$select = db_select('node', 'n')
->fields('n', array('title'))
->condition('n.nid', $node->book['bid'])
->addTag('node_access');

$title = $select->execute()->fetchField();
...

Note to maintainers: Thank you for your efforts in writing the code and maintaining this log. It's a great help!

neoglez’s picture

Status: Closed (fixed) » Needs review
FileSize
602 bytes

I could open another issue but since the title is also valid for this one i'm posting the patch here.
Patch described in #15, this time for book module, it's of the same nature of patch in #12 (commited), i tested & it seems to work fine.

neoglez’s picture

Priority: Normal » Major

Mm, by the way, i think this is major:

Issues which have significant repercussions but do not render the whole system unusable are marked major. An example would be a PHP error which is only triggered under rare circumstances or which affects only a small percentage of all users. These issues are prioritized in the current development release and backported to stable releases where applicable. Major issues do not block point releases.

marcingy’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Needs to be fixed in d8 first.

andypost’s picture

Are this affected by "node access queries to 7.3" ? http://drupal.org/node/1204572

catch’s picture

hswong3i’s picture

Status: Needs review » Reviewed & tested by the community

Confirm patch from #16 can apply to both D7/8 and able to solve the problem with node access.

webchick’s picture

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

I had the other issue tagged needs tests, because we need them. The issue that got marked duplicate had a patch committed that caused the problem that this patch is fixing, because it didn't come with tests in the first place.

neoglez’s picture

(...) because it didn't come with tests in the first place.

You mean that the people should test it or write a test for this use case??
...couldn't find a description of "Needs test".

webchick’s picture

Sorry, they should write an automated test for this use case. Something that would hit this section of code and cause the test to fail without the patch, and passes with it.

salvis’s picture

Marked #1227860: PDOException: Integrity constraint violation: ambiguous nid in book.module with node_access as a duplicate of this.

There are other similar cases, e.g. in blog.module, forum.module, even node.module, with unqualified column names like nid/uid/status/etc. in condition() calls.

@webchick: Once we have the table alias in place it's unlikely to get lost again — I'm not sure just how worthwhile a test is here, but in order to not derail this issue I've created #1228214: Database queries should refuse unqualified column names in condition() clauses or default to the base table, which proposes a more effective approach.

deggertsen’s picture

Patch in #16 fixed this problem on my site. Thanks!

rbayliss’s picture

@webchick: In this case, you have a module providing node grants in order to trigger this error. I don't see how we can write a test case for this as there are no node_grants providers in core (or am I wrong?).

andypost’s picture

Core already has node_access_test.module, suppose this should be extended a bit

Anonymous’s picture

I have marked #1057770: SQL error when trying to view book as duplicate of this report.

AdamGS’s picture

subscribe

Jeremy’s picture

Subscribing, as was reported against the support module #1249788: unexpected error in book module when enabling support ticketing module in 7.x but is a bug in the book module.

dwalker51’s picture

patch at #16 worked for me.

neoglez’s picture

Component: menu system » database system
Status: Needs work » Needs review
FileSize
3.2 KB

@webnick here is the test.
As usual there are more than one approach but i think this one of the less painfull.
By applying this patch to the test one can run the two cases: with patch in #16 and without and see the exceptions (beautifully) thrown.
The patch is against D8.

Status: Needs review » Needs work

The last submitted patch, test-book-block-on-access-module.patch, failed testing.

neoglez’s picture

The last submitted patch, test-book-block-on-access-module.patch, failed testing.

Well, that's the expected result, isn't it? ;-)
View details for more info.

salvis’s picture

Exactly. Now post a patch with #16 and #33 combined; and set 'needs review' again.

sarah_p’s picture

Status: Needs work » Needs review
FileSize
3.39 KB

This is a merged version of the patches in 16 and 33.

Status: Needs review » Needs work

The last submitted patch, patch16n33.patch, failed testing.

neoglez’s picture

mm, at least we don't have more exceptions.
I see the reason of the fail in BookTestCase->testNavigationBlockOnAccessModuleEnabled() is becouse i forgot to give anonymous users the 'node test view' permission, the other two fails in BookTestCase->testBookNavigationBlock() are 'new'.
@sarah_p Thanks for the merge!

neoglez’s picture

Status: Needs review » Needs work

Aha! The $this->admin_user needs also 'node test view' permission, something like:
- $this->admin_user = $this->drupalCreateUser(array('create new books', 'create book content', 'edit own book content', 'add content to books', 'administer blocks'));
+ $this->admin_user = $this->drupalCreateUser(array('create new books', 'create book content', 'edit own book content', 'add content to books', 'administer blocks', 'node test view'));

neoglez’s picture

Well, actually no.
Let's try this one althoug i think it might make sense to implement another class to (really) separate the cases when an access module is enabled.

neoglez’s picture

Status: Needs work » Needs review

Update status

Status: Needs review » Needs work

The last submitted patch, test-book-block-on-access-module-1.patch, failed testing.

neoglez’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

Oops...

The last submitted patch, test-book-block-on-access-module-2.patch, failed testing.

neoglez’s picture

neoglez’s picture

Status: Needs work » Needs review
andypost’s picture

Status: Needs review » Reviewed & tested by the community

I think this patch is great to be commited, the whitespace can be fixed latter

+++ b/modules/book/book.testundefined
@@ -274,6 +277,12 @@ class BookTestCase extends DrupalWebTestCase {
+     ¶

A trailing whitespace

chx’s picture

Or we can remove that. I also have attached the test only version to see it failing.

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.03 KB

Comments weren't up to coding standards, here's an edited patch, if this is RTBC again when I'm back off my flight tomorrow I'm happy to commit otherwise.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Sure it is.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome. Great work on this folks! I'm sorry about being a stickler about tests, but hopefully this stays fixed now. :)

Committed and pushed to 8.x and 7.x. Yay for major bugs biting the dust! :)

neoglez’s picture

Thanks for the acknowledgement!! ...very motivating :)

webchick’s picture

No problem at all! Thanks for the patch! :D

embeepea’s picture

I'm not sure if this is related to this issue or not, but I'm seeing a very similar ambiguous 'nid' column error using D7.8 with
Entity Reference 7.x-1.0-beta1, together with either Organic Groups Access Control (7.x-1.1) or Content Access (7.x-1.2-beta1). I am not using the Book module at all. The error occurrs when either of these content access modules is enabled, but does not happen when both are disabled. #1309704: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 has more details. I just though I'd post a note here too in case anyone from this discussion can offer any insights, since the issue seems similar.

webchick’s picture

can you see if you get the same error with 7.x-dev?

neoglez’s picture

@mark.phillips the same problem described in this issue appears (apparently) all over core (see #25 and specially #1228214: Database queries should refuse unqualified column names in condition() clauses or default to the base table), but at least for this specific case (book module) the issue should be resolved in 7.x and 8.x (#49).

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -Needs backport to D7

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

deanflory’s picture

Issue summary: View changes

Resaving the User: name field in Views worked for me to get beyond the bug in D7.27.