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.I imagine this harks back to earlier implementations, but the current name and description of this option are not appropriate.
It says:
Disabling SQL rewriting will disable node_access checks as well as other modules that implement hook_query_alter().
However this option does not disable "SQL rewriting" and it does not affect any hook_query_alter() implementation which does not target the "access query tag" for the View in question.
What it actually does is prevent the query being tagged with whatever was configured as the "access query tag" in the base table for the View, as defined in hook_views_data().
e.g. in node_views_data() we see:
$data['node']['table']['base'] = array(
'field' => 'nid',
'title' => t('Content'),
'weight' => -10,
'access query tag' => 'node_access',
'defaults' => array(
'field' => 'title',
),
);
views_plugin_query_default::execute() and views_handler_relationship::execute() then simply do $query->addTag($access_tag);, and standard tag-based query alteration takes over from there.
There's one hard-coded case in views_handler_relationship_node_term_data::query() where $query->addTag('term_access'); is set explicitly, but I believe that's entirely in line with the more generic cases.
So both the name and the description of the option are misleading, and it's very easy to be concerned that you would disable more functionality than you actually wish to.
e.g. In my case I wanted to disable node access (only) for a view, and I originally ignored this option on the basis that it clearly did far more than that, when in fact it was exactly what I wanted.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | views-2226015-3.21.patch | 2.5 KB | RoSk0 |
| #14 | views-disable_sql_rewrite-2226015-14.patch | 2.5 KB | jweowu |
| |||
Comments
Comment #1
jweowu CreditAttribution: jweowu commentedPatch with proposed replacement text.
Comment #2
jweowu CreditAttribution: jweowu commentedA good improvement might be to look up the base table "access query tag" for the View being edited, and to present that value in the UI text so that the option can indicate precisely which tag it is going to affect.
Comment #3
jweowu CreditAttribution: jweowu commentedComment #4
yang_yi_cn CreditAttribution: yang_yi_cn commentedI think this is a pretty good change. I spent quite some time today looking at ways to disable node access in views but not disable hook_query_alter(), because I use hook_query_alter() to change select query to use slave database. Just realized that checkbox doesn't affect hook_query_alter() at all, the text is really misleading.
Comment #5
jweowu CreditAttribution: jweowu commentedThat's very similar to what happened to me. I took the text at face value; and while I didn't have a specific hook_query_alter that I was worried about, I certainly didn't want to prevent them from running if any were added subsequently (either custom or contrib).
It was quite a while before it occurred to me to dig into the code and find out exactly what effect it had (and slightly frustrating to discover that the option had actually done exactly what I wanted it to all along).
Can we call this RTBC?
Comment #6
jweowu CreditAttribution: jweowu commentedComment #7
jweowu CreditAttribution: jweowu commented(Oh good grief. Please ignore the utterly irrelevant patch for the diff module. I'm not sure how to delete it.)
This patch implements my suggestion from #2, and now makes explicit which query tag will be omitted if the "Disable access checks" option is used.
I'm omitting this option from the query options form altogether if no access query tag is found for the base table. As this option has no effect when there is no access query tag for the base table, I think this is a reasonable thing to do.
The only potential issue I could think of was if there were a way to modify the base table or access query tag for a View in a way that took effect during execution, but not during administration of the View. I'm pretty certain that's a non-issue?
Comment #8
jweowu CreditAttribution: jweowu commentedComment #10
jweowu CreditAttribution: jweowu commentedRe-setting status after accidentally misleading the test bot.
Comment #11
jweowu CreditAttribution: jweowu commentedTidied up the warning message.
Comment #12
jweowu CreditAttribution: jweowu commentedyang_yi_cn: Would you be willing to review the new patch in #11 ?
I think this is by far the nicer of the two approaches, so it would be good to get another pair of eyes on it, and maybe move it to RTBC if you feel it's good to go.
Comment #13
RoSk0Good improvement to UI, thanks @jweowu.
Comment #14
jweowu CreditAttribution: jweowu at Catalyst IT commentedRe-roll against current HEAD. Could this please be applied?
Comment #15
jweowu CreditAttribution: jweowu at Catalyst IT commentedComment #17
DamienMcKennaCommitted. Thanks.
Comment #19
RoSk0Re-roll of #14 on top 3.21 version.