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.
Problem/Motivation
"Views query alter should respect the "Disable SQL Rewriting" setting"
The condition to respect option wasn't added in taxonomy's views relationship query table, therefore the tag "taxonomy_term_access" was added regardless of $query->options['disable_sql_rewrite'] option set to TRUE or FALSE. This was mentioned in 3007949.
Steps to reproduce
- Create new view for contents
- Add new relationship "Taxonomy terms on node"
- Check any vocabulary you have in your taxonomy
- Set breakpoint within core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData::query.
- You should see it being adding to the query table.
Proposed resolution
- Add condition empty($this->query->options['disable_sql_rewrite']) to $query->addTag('taxonomy_term_access").
$query->condition('td.vid', array_filter($this->options['vids']), 'IN');
if (empty($this->query->options['disable_sql_rewrite'])) {
$query->addTag('taxonomy_term_access');
}
$query->fields('td');
Remaining tasks
Needs Tests
Needs Review
Original report by [username]
Adding
$query->addTag('term_access');
unconditionally for a view with taxonomy relationship is discourraged.
See attached patch.
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff-30-34.txt | 1.56 KB | g-brodiei |
#34 | 2132773-no-tag-please-34.patch | 3.41 KB | g-brodiei |
#34 | 2132773-no-tag-please-failtest-5.patch | 2.52 KB | g-brodiei |
#30 | 2132773-no-tag-please-30.patch | 3.22 KB | g-brodiei |
#30 | 2132773-no-tag-please-failtest-4.patch | 2.33 KB | g-brodiei |
Comments
Comment #1
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedComment #2
dawehnerThank you. Committed and pushed.
Comment #3
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedAttaching patch for d8.
Comment #4
dawehnerThe coe looks perfect, though I assume that people would require some sort of test coverage for that.
Comment #5
damiankloip CreditAttribution: damiankloip commentedI will add some test coverage for this.
Comment #6
mr.baileysComment #15
g-brodieiI will work on this test coverage.
Comment #16
clemens.tolboomComment #17
g-brodieiAdding tag in query should respect the override of "disable_sql_rewrite" option settings as mentioned in 3007949.
Comment #19
g-brodieiSet back to need review since it was meant to fail in the first place.
Comment #20
jonathanshawA lovely simple issue! Nice work @g-brodiei
I confirmed that this is what we do in the parent class:
Drupal\views\Plugin\views\relationship\RelationshipPluginBase
but because we're overriding the parent method we need to reimplement that check here. So the idea looks good to me.
Why not just empty($query->options ...
Out of scope. If it's not necessary to fix here, don't. There's probably some other mega patch going on to change these and you'll just cause collision with it.
Comment #21
g-brodieiThanks for the thorough explanation @jonathanshaw!! Spent some time wrapping my head around that fail test to fix hahaha.
Alright, second try!
Thanks again!!
Comment #23
jonathanshawI'm afraid you're going to need to test the view with AND without the disable_sql_rewrite. Which means you're probably going to need an assertViewHasTermAccessTag() helper method.
Searching the core codebase, I see no instances of a null coalescing operator being used outside of variable assignment. I've asked about this on slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1599646560133800
Comment #24
jonathanshaw@berdir said on slack:
"you don't need ?? with empty() and using it to actually run code and not just have a fallback value to assign is definitely not something I've seen in core. It's basically an inline if without {}, which we don't allow, so I'd write this as a regular if () with curly braces"
Comment #25
g-brodieiHi jonathanshaw,
No problem, let me get this straight first before I make another patch again.
So this means I should be testing the view while the ['option']['disable_sql_rewrite'] removed and the test should fail when going through a custom assertViewHasTermAccessTag() helper method (I'll figure how to write this out).
What I'm not getting is that when "disable_sql_rewrite" is FALSE or not set (the default is FALSE), the "taxonomy_term_access" tag is being added anyway, then the test without patch won't fail with assertViewHasTermAccessTag(). (I thought we want to test it to fail when the patch condition wasn't added)?
2. For the Null Coalescing Operator, I'll revert this back to it's original state with if(empty()){}, if I'm understanding this correctly, thanks!
Comment #26
jonathanshawSometimes code says it clearer than words ... I'd expecting to see something along the lines of this:
We're testing the view without AND without disable_sql_rewrite, ensuring that the tag is set by default, but not set when disable_sql_rewrite is set.
The test only patch is simply a subset of the main patch (i.e. identical code the main patch) but it includes only the changes to RelationshipNodeTermDataTest. When this test-only patch runs on Drupal CI, it will fail, showing that the test does expose the bug that this issue is about. And therefore when the main patch passes on Drupal CI, we know that the solution in the patch does actually solve the problem here. Otherwise for all we know all the code could be completely pointless, doing nothing.
The Issue Summary does need some expansion too.
Comment #27
g-brodieiWow, thanks for spending so much time and trouble explaining this, I really appreciate the help and mentoring!
Yes, the code was much easier to understand, probably it's my English problem, Hahaha (my teacher must be disappointed). I never thought about using assertSame(), thanks for this demo, makes me think out of the scope once and for all, very cool indeed.
Following your suggestions, I've also made some adjustments while testing it with phpunit.
1. A new $view object for the test, from how it was done in #3007949: Views query alter should respect the "Disable SQL Rewriting" setting.
2. executeView() so it retrieves the query table.
Comment #28
g-brodieiComment #29
jonathanshawNice! A few very small cosmetic details and we're finished.
Technically this doc improvement is out of scope. Best not to do this.
I'd say "Assert views queries have taxononomy_term_access tag".
We can typehint with Drupal\views\View. Then the comment could be "The View to check for the term access tag".
You can remove TRUE of FALSE, we've said it's bool. No capital T on Tag, as it's not the name of a class.
Comment #30
g-brodieiCool, almost there, almost there, last mile to run!!!! Thanks for leading ahead~
Here is the change I've done.
1. Removed the function comment, which is out of scope.
2. Created a new test as testTag() for cleaner assertion on what we are doing.
3. Typehint the helper function $view variable as (\Drupal\views\ViewExecutable $view, $hasTag). which is the expected object type of $view after $view = View:getView('test_taxonomy_node_term_data');.
Uploading test patch, fail-test patch, and interdiff.txt between 27-30.
Comment #31
g-brodieiBack to need review!
Comment #32
jonathanshawThe main reason I can't set this to RTBC is that it "Needs issue summary update" becaus the current issue summary is too brief.
I'm afraid I noticed a few more things when doing a final review:
The method documentation should describe what the test does. It certainly doesn't test the whole of NodeTermData::query(). Maybe "Tests that the 'taxonomy_term_access' tag is added to the Views query." is enough.
No blank lines at start of method.
I'm not clear why this is necessary or desirable? If it is necessary, then probably we should be consistent with the way the $display_options are altered below.
Comment #33
g-brodieiUpdated issue summary. Removed Need Summary Update Tag.
No idea what the tag VDC stands for???
Yes, this setting was required since the view config file "test_taxonomy_node_term_data" that this test used doesn't have any relationship tag setup in the first place. This is the requirement setup to allow the query function to add the tag, we need to add this part before running the assertion of disable_sql_rewrite is TRUE or FALSE.
I don't think it is necessary to run this config setup line again when we alter the disable_sql_rewrite option to TRUE?
Comment #34
g-brodieiFix base on suggestions.
1. Adjust the comment block for testTag().
2. Removed extra line at start of method.
3. Fixed coding standards "Namespaced classes/interfaces/traits should be referenced with use statements".
Comment #36
jonathanshawNice :)
Comment #38
catchCommitted 50e0c6f and pushed to 9.1.x. Cherry-picked back to 8.9.x, thanks!