Problem/Motivation

#1204658: Always use query metadata to specify the node access base table removes the base table fallback for node access queries. However, there is no documentation indicating how to specify the base table.

Proposed resolution

Add documentation on _node_query_node_access_alter() indicating that queries tagged with node access that are not against the node table should add the base table to the query metadata, e.g.:

 * @code
 *   $query
 *     ->addTag('node_access')
 *     ->addMetaData('base_table', 'taxonomy_index');
 * @endcode

For Drupal 7, additionally add that the base table fallback is deprecated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: +Novice
xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Um. I read the issue summary here and got confused. It looks like you are saying "if it's not against the node table, you need the tag" for D8, but then it also looks like you are saying there is no default table in d8 because the fallback was removed? Or is "fallback removed" talking about something else?

xjm’s picture

The linked issue should explain.

xjm’s picture

Okay, a brief summary of the situtation, since it's mostly undocumented (which is why I filed this issue, after all):

  • The node access system requires developers to tag direct queries that return node-related data with the node_access tag so access control can be applied.
  • If the tagged query is against a table other than the {node} table, node_query_node_access_alter() needs to know what the base table of the query is.
  • The normal way to indicate the base table is to add the metadata to the query, as in the example in the summary.
  • Drupal 7 includes a mechanism to "guess" what the base table is--the base table fallback. In Drupal 8, this has been removed in #1204658: Always use query metadata to specify the node access base table. In Drupal 7, the fallback has not been removed, but it should be considered deprecated.
jhodgdon’s picture

Ah, OK. So:
- In D7/8, we need to document how to tell the node query alter what the base table is. ->addMetaData('base_table', 'taxonomy_index');
- In Drupal 8, we need to document that if the table is not {node}, you MUST use this.
- In Drupal 7, we need to document that if the table is not {node}, an attempt is made to guess the table, but that we don't recommend relying on this since it's going away in Drupal 8, and it's probably safer anyway just to provide the table.

xjm’s picture

Yep, precisely. Thanks @jhodgdon!

kim.pepper’s picture

Status: Active » Needs review
FileSize
854 bytes

Here's a documentation patch for D8.

Kim

xjm’s picture

This looks great. I've a couple minor suggestions:

+++ b/core/modules/node/node.moduleundefined
@@ -2890,6 +2890,14 @@ function node_access_view_all_nodes($account = NULL) {
+ * Important: Queries tagged with node_access that are not against the node
+ * table should add the base table as metadata. For example:

We can probably remove the word "Important" here, I think. I'd also maybe add single quotes around 'node_access' and curlies around {node}.

kim.pepper’s picture

FileSize
847 bytes

Thanks for the feedback, xjm.

Changes applied.

xjm’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I think we need to replace "should" with "must" for Drupal 8, right?

kim.pepper’s picture

Replaced "should" with "must".

kim.pepper’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That works for me, thanks! I'll get it committed shortly.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

That patch is committed to 8.x, thanks again!

We need a slightly different patch for 7.x -- see #5 (which doesn't mean "use the wording in #5", but that says what needs to be documented).

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

cs_shadow’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
866 bytes

Attaching patch for D7. Any suggestions are welcome.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! There is a spelling error: "alwasys". Other than that, it seems fine to me.

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
865 bytes

Fixed the spelling mistake.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

By the way, you've been doing several patching issues... When a patch is reviewed and marked "needs work", and you provide a new patch, it is very helpful to provide an "interdiff" file, so the reviewers can easily see what you changed. See https://drupal.org/documentation/git/interdiff (not necessary for this issue, but going forward, please do). Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

  • Commit 27304fc on 7.x by jhodgdon:
    Issue #1885420 by cs_shadow, kim.pepper, xjm: Fix docs for metadata on...
cs_shadow’s picture

@jhodgdon, I'll definitely keep that in mind in future. Thanks!

Status: Fixed » Closed (fixed)

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