Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.We have issues such as #766382: Column 'nid' is ambiguous when using node access modules which is waiting for a test to be written (and surely others that are still waiting to be discovered and diagnosed) while core remains broken. The problem in that specific case is a query in book.module...
$select = db_select('node', 'n')
->fields('n', array('title'))
->condition('nid', $node->book['bid'])
->addTag('node_access');
... namely that the field parameter in the condition() call is unqualified. It should be
->condition('n.nid', $node->book['bid'])
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, and I'm sure there are hundreds more in contrib.
We can try to fix these one by one, but they probably grow faster than we can fix them. I think it would make sense to require the field parameter to contain a qualified field name, with a table name or alias, at least for queries that are intended to be modified (addTag('node_access')), but really for all queries.
Writing tests for node_access core issues is tedious, because AFAIK there's no node_access module in core — you'd have to write one for that specific test. Maybe it could be reused, but we need to test not only for ambiguous nid, but also for ambiguous uid, ambiguous status, etc.
The same goes for orderBy() and groupBy(), BTW. This needs to be fixed in DBTNG, not with tests.
(There's even bad documentation such as the hook_user_cancel sample code.)
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell. This could actually be considered a bug of db_select(): a unqualified field could be automatically prefixed by the main table alias.
Comment #2
salvisNo, it's not that easy. For a join query, the field could come from any of the tables, as long as it's unambiguous (in the unaltered query).
EDIT: But I agree that it could be fixed up automatically and thus fixed in D7 by looking for the right table.
Comment #3
neoglez CreditAttribution: neoglez commentedYes, in deed!
well, there is in core node_test_access
Comment #4
neoglez CreditAttribution: neoglez commentedComment #5
salvisLook at the size of the patch in #766382: Column 'nid' is ambiguous when using node access modules (for adding two characters!) and the effort it took to complete it.
How many similar cases do we have in core? Maybe 50? So, multiply by 50...
If the two characters had been there from the start, no one would ever have missed that new test.
Comment #14
catchThis seems sensible, would need to deprecate the current behaviour first, then enforce it in a major release. Moving to a task though since this is an API change. Would also need to fix core before committing anything here, although a patch would help to flush out the cases.