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.
Comment | File | Size | Author |
---|---|---|---|
#18 | doc-1885420-18.patch | 865 bytes | cs_shadow |
#16 | doc-1885420-16.patch | 866 bytes | cs_shadow |
#12 | doc-base-table-1885420-12.patch | 845 bytes | kim.pepper |
#9 | doc-base-table-1885420-9.patch | 847 bytes | kim.pepper |
#7 | doc-base-table-1885420-7.patch | 854 bytes | kim.pepper |
Comments
Comment #1
xjmComment #1.0
xjmUpdated issue summary.
Comment #2
jhodgdonUm. 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?
Comment #3
xjmThe linked issue should explain.
Comment #4
xjmOkay, a brief summary of the situtation, since it's mostly undocumented (which is why I filed this issue, after all):
node_access
tag so access control can be applied.{node}
table,node_query_node_access_alter()
needs to know what the base table of the query is.Comment #5
jhodgdonAh, 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.
Comment #6
xjmYep, precisely. Thanks @jhodgdon!
Comment #7
kim.pepperHere's a documentation patch for D8.
Kim
Comment #8
xjmThis looks great. I've a couple minor suggestions:
We can probably remove the word "Important" here, I think. I'd also maybe add single quotes around
'node_access'
and curlies around{node}
.Comment #9
kim.pepperThanks for the feedback, xjm.
Changes applied.
Comment #10
xjmThanks, looks good to me!
Comment #11
jhodgdonI think we need to replace "should" with "must" for Drupal 8, right?
Comment #12
kim.pepperReplaced "should" with "must".
Comment #13
kim.pepperComment #14
jhodgdonThat works for me, thanks! I'll get it committed shortly.
Comment #15
jhodgdonThat 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).
Comment #15.0
jhodgdonUpdated issue summary.
Comment #16
cs_shadow CreditAttribution: cs_shadow commentedAttaching patch for D7. Any suggestions are welcome.
Comment #17
jhodgdonThanks! There is a spelling error: "alwasys". Other than that, it seems fine to me.
Comment #18
cs_shadow CreditAttribution: cs_shadow commentedFixed the spelling mistake.
Comment #19
jhodgdonLooks 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!
Comment #20
jhodgdonThanks again! Committed to 7.x.
Comment #22
cs_shadow CreditAttribution: cs_shadow commented@jhodgdon, I'll definitely keep that in mind in future. Thanks!