Note: I don't have time at the moment to isolate all the variables in this report, but I want to get something out there and throw a patch at the testbot. In the meantime, here's what (I think) I know:

If you have a node with revisioning enabled and you perform a node access respecting query for it (e.g., by viewing the node display page at node/nid), it causes an uncaught exception:

Query tagged for node access but there is no node table, specify the base_table using meta data.

This seems to be because the base table selection logic in node_query_node_access_alter() doesn't account for revisioning. In Drupal 7, said logic checked the query table for any fields with foreign keys of node.nid, whereas in Drupal 8 it only looks for {node} and {node_field_data}. But a node with revisions will use the {node_revision} table. I am admittedly no expert with node access, but it seems (in my limited testing) to work if you just add that table to the list of fallbacks. I'll follow with a patch to see what the testbot says.

CommentFileSizeAuthor
#11 10-11-interdiff.txt1.77 KBalexpott
#11 2664748-11.patch2.65 KBalexpott
#10 interdiff.txt1.94 KBamateescu
#10 2664748-10.patch3.01 KBamateescu
#8 interdiff.txt572 bytesamateescu
#8 2664748-8.patch2.1 KBamateescu
#5 2664748-5.patch2.1 KBamateescu
#5 2664748-5-test-only.patch1.03 KBamateescu
#2 node_revision_queries-2664748-2.patch635 bytesTravisCarden
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden created an issue. See original summary.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden at Acquia commented
Assigned: TravisCarden » Unassigned
Status: Active » Needs review
FileSize
node_revision_queries-2664748-2.patch635 bytes
TheodorosPloumis’s picture

TheodorosPloumis
he/him
Location Greece
CreditAttribution: TheodorosPloumis commented

I had the same problem with the workbench_moderation module and this patch fixes the error.

zerolab’s picture

zerolab CreditAttribution: zerolab at Torchbox commented

Had the same issue described in #1 and #3. The patch sorted out the exception.
The patch is straightforward, but still requires someone with more in-depth knowledge of the node access system in Drupal 8 to cast an eye.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2664748-5-test-only.patch1.03 KB
2664748-5.patch2.1 KB 
+++ b/core/modules/node/node.module
@@ -1052,7 +1052,7 @@ function node_query_node_access_alter(AlterableInterface $query) {
+        if (in_array($table, ['node', 'node_revision', 'node_field_data'])) {

We also need to take the revision data table into account ('node_field_revision'), but it's easier to just get the full list of base tables from the table mapping.

Here's an updated patch with that change and a test.

The last submitted patch, 5: 2664748-5-test-only.patch, failed testing.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden at Acquia commented
Status: Needs review » Reviewed & tested by the community

That looks great, @amateescu! Thanks!

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2664748-8.patch2.1 KB
interdiff.txt572 bytes

Just a small update for coding standards in tests :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/node/node.module
@@ -1049,11 +1049,15 @@ function node_query_node_access_alter(AlterableInterface $query) {
   // If the base table is not given, default to node if present.

This comment seems out of date - and also should we prefer node or node_field_data if it is present? I can image there is a performance benefit to joining to them over a revisions table. D7 seems to have this kind of optimisation. 'node' is preferred to all others.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
2664748-10.patch3.01 KB
interdiff.txt1.94 KB

How about something like this?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2664748-11.patch2.65 KB
10-11-interdiff.txt1.77 KB

I think there's no difference between node and node_field_data in terms of join performance to node_access as they both only have a single row per node... maybe node's primary key of PRIMARY KEY (`nid`), will be quicker than node_field_data's PRIMARY KEY (`nid`,`langcode`),. But I would expect it to be much quicker that joining to the revisions where there are multiple rows for each node id.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Nice, that looks much better!

  • catch committed cc88102 on 8.2.x 
    Issue #2664748 by amateescu, alexpott, TravisCarden: Node revision...

  • catch committed cbf6494 on 8.1.x 
    Issue #2664748 by amateescu, alexpott, TravisCarden: Node revision...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches, thanks!

  • catch committed 7e0cdea on 8.0.x 
    Issue #2664748 by amateescu, alexpott, TravisCarden: Node revision...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Status: Fixed » Closed (fixed)

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