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

  1. Create new view for contents
  2. Add new relationship "Taxonomy terms on node"
  3. Check any vocabulary you have in your taxonomy
  4. Set breakpoint within core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData::query.
  5. You should see it being adding to the query table.

Proposed resolution

  1. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

killes@www.drop.org’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
930 bytes
dawehner’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 7.x-3.x-dev » 8.x-dev
Component: taxonomy data » ajax system
Status: Needs review » Patch (to be ported)

Thank you. Committed and pushed.

Sivaji_Ganesh_Jojodae’s picture

Status: Patch (to be ported) » Needs review
Issue tags: +Needs issue summary update
FileSize
915 bytes

Attaching patch for d8.

dawehner’s picture

Issue tags: +VDC

The coe looks perfect, though I assume that people would require some sort of test coverage for that.

damiankloip’s picture

I will add some test coverage for this.

mr.baileys’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

g-brodiei’s picture

Assigned: Unassigned » g-brodiei
Issue tags: +Bug Smash Initiative

I will work on this test coverage.

clemens.tolboom’s picture

Component: ajax system » taxonomy.module
g-brodiei’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
3.07 KB
3.39 KB
1.55 KB

Adding tag in query should respect the override of "disable_sql_rewrite" option settings as mentioned in 3007949.

  1. Create fail test without patch, testing it to fail if tag still exists in query.
  2. Updated test on Query to 9.1.x by extending the current test in RelationshipNodeTermDataTest.php.
  3. Updated method "assertIdentical()" to "assertSame()" for deprecation reason.

Status: Needs review » Needs work

The last submitted patch, 17: 2132773.no-tag-please-failtest.patch, failed testing. View results

g-brodiei’s picture

Status: Needs work » Needs review

Set back to need review since it was meant to fail in the first place.

jonathanshaw’s picture

Status: Needs review » Needs work

A 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

    // Add access tags if the base table provide it.
    if (empty($this->query->options['disable_sql_rewrite']) && isset($table_data['table']['base']['access query tag'])) {
      $access_tag = $table_data['table']['base']['access query tag'];
      $this->query->addTag($access_tag);
    }

but because we're overriding the parent method we need to reimplement that check here. So the idea looks good to me.

  1. The test only patch has the opposite assertion to the test patch. The whole point of the test only patch is to have the same assertion, but not the fix.
  2. The bigger problem is that the test coverage, while good, doesn't actually test for the problem of this issue (that the tag is not added if sql rewriting is disabled). The tests in #3007949: Views query alter should respect the "Disable SQL Rewriting" setting show what is needed though, so should be easy to add.
  3. +++ b/core/modules/taxonomy/src/Plugin/views/relationship/NodeTermData.php
    @@ -133,7 +133,9 @@ public function query() {
    +      if (empty($this->query->options['disable_sql_rewrite'])) {
    

    Why not just empty($query->options ...

  4. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -23,6 +23,9 @@ class RelationshipNodeTermDataTest extends TaxonomyTestBase {
    +  /**
    +   * Tests Views Relationship Handler NodeTermData.
    +   */
    
    @@ -34,7 +37,7 @@ public function testViewsHandlerRelationshipNodeTermData() {
    -    $this->assertIdentical($expected, $view->getDependencies());
    +    $this->assertSame($expected, $view->getDependencies());
    
    @@ -55,9 +58,23 @@ public function testViewsHandlerRelationshipNodeTermData() {
    -    $this->assertIdentical($expected, $view->getDependencies());
    +    $this->assertSame($expected, $view->getDependencies());
    

    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.

g-brodiei’s picture

Thanks for the thorough explanation @jonathanshaw!! Spent some time wrapping my head around that fail test to fix hahaha.

Alright, second try!

  1. Update test by testing to have no "taxonomy_term_access" tag to exist when setting "disable_sql_rewrite" to TRUE. Which should fail since the original code does not have this condition included.
  2. Removed out of scope of deprecation assertion method to avoid code collision.
  3. Change condition to shorthand Null Coalescing Operator.

Thanks again!!

The last submitted patch, 21: 2132773-no-tag-please-failtest-2.patch, failed testing. View results

jonathanshaw’s picture

Status: Needs review » Needs work

Update test by testing to have no "taxonomy_term_access" tag to exist when setting "disable_sql_rewrite" to TRUE. Which should fail since the original code does not have this condition included.

I'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.

Change condition to shorthand Null Coalescing Operator.

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

jonathanshaw’s picture

@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"

g-brodiei’s picture

Hi jonathanshaw,

No problem, let me get this straight first before I make another patch again.

I'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.

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!

jonathanshaw’s picture

Sometimes code says it clearer than words ... I'd expecting to see something along the lines of this:

  $this->assertIdentical($expected, $view->getDependencies());
  $this->executeView($view, [$this->term1->id(), $this->term2->id()]);
  $this->assertIdenticalResultset($view, $expected_result, $column_map);

  // By default, the view has the term_access tag.
  $this->assertQueriesTermAccessTag($view, TRUE);

  // The term_access tag is not set if disable_sql_rewrite is set.
  $display = $view->getDisplay();
  $display_options = $display->getOption('query');
  $display_options['options']['disable_sql_rewrite'] = TRUE;
  $display->setOption('query', $display_options);
  $view->save();
  $this->assertQueriesTermAccessTag($view, FALSE);
}

protected function assertQueriesTermAccessTag($view, $hasTag) {

  $main_query = $view->build_info['query'];
  $count_query = $view->build_info['count_query'];

  // Tests \Drupal\taxonomy\Plugin\views\relationship\NodeTermData::query().
  foreach ([$main_query, $count_query] as $query) {
     $tables = $query->getTables();
     foreach ($tables as $join_table) {
        if (is_object($join_table['table'])) {
         $this->assertSame($join_table['table']->hasTag('taxonomy_term_access'), $hasTag);
      }
    }
}

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.

g-brodiei’s picture

Wow, 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.

    $view = Views::getView('test_taxonomy_node_term_data');

2. executeView() so it retrieves the query table.

    $display->setOption('query', $display_options);
    $view->save();
    $this->executeView($view, [$this->term1->id(), $this->term2->id()]);
g-brodiei’s picture

Status: Needs work » Needs review
jonathanshaw’s picture

Status: Needs review » Needs work

Nice! A few very small cosmetic details and we're finished.

  1. It looks like the test you've added can now be moved to a seperate test method of its own? Much neater that way if possible. e.g. public function testTag()
  2. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -23,6 +23,9 @@ class RelationshipNodeTermDataTest extends TaxonomyTestBase {
    +  /**
    +   * Tests Views Relationship Handler NodeTermData.
    +   */
    

    Technically this doc improvement is out of scope. Best not to do this.

  3. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -58,6 +61,43 @@ public function testViewsHandlerRelationshipNodeTermData() {
    +   * Asserts Views Query Tables for taxonomy_term_access Tag Existence.
    

    I'd say "Assert views queries have taxononomy_term_access tag".

  4. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -58,6 +61,43 @@ public function testViewsHandlerRelationshipNodeTermData() {
    +   * @param object $view
    +   *   An executable object from Drupal\views\Views.
    ...
    +  protected function assertQueriesTermAccessTag($view, $hasTag) {
    

    We can typehint with Drupal\views\View. Then the comment could be "The View to check for the term access tag".

  5. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -58,6 +61,43 @@ public function testViewsHandlerRelationshipNodeTermData() {
    +   *   The expected existence of taxonomy_term_access Tag, TRUE or FALSE.
    

    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.

g-brodiei’s picture

Cool, 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.

-  /**
-   * Tests Views Relationship Handler NodeTermData.
-   */

2. Created a new test as testTag() for cleaner assertion on what we are doing.

  /**
   * Tests \Drupal\taxonomy\Plugin\views\relationship\NodeTermData::query().
   */
  public function testTag() {

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.

g-brodiei’s picture

Status: Needs work » Needs review

Back to need review!

jonathanshaw’s picture

Issue tags: -Needs tests

The 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:

  1. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -60,4 +60,53 @@ public function testViewsHandlerRelationshipNodeTermData() {
    +  /**
    +   * Tests \Drupal\taxonomy\Plugin\views\relationship\NodeTermData::query().
    +   */
    +  public function testTag() {
    

    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.

  2. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -60,4 +60,53 @@ public function testViewsHandlerRelationshipNodeTermData() {
    +
    

    No blank lines at start of method.

  3. +++ b/core/modules/taxonomy/tests/src/Functional/Views/RelationshipNodeTermDataTest.php
    @@ -60,4 +60,53 @@ public function testViewsHandlerRelationshipNodeTermData() {
    +    // Change the view to test relation limited by vocabulary.
    +    $this->config('views.view.test_taxonomy_node_term_data')
    +      ->set('display.default.display_options.relationships.term_node_tid.vids', ['views_testing_tags'])
    +      ->save();
    

    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.

  4. The coding standards tests https://www.drupal.org/pift-ci-job/1820133 alerted me to 1 problem that needs fixing on line 98
g-brodiei’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated 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?

@@ -60,4 +60,53 @@ public function testViewsHandlerRelationshipNodeTermData() {
+    // Change the view to test relation limited by vocabulary.
+    $this->config('views.view.test_taxonomy_node_term_data')
+      ->set('display.default.display_options.relationships.term_node_tid.vids', ['views_testing_tags'])
+      ->save();
g-brodiei’s picture

Fix 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".

The last submitted patch, 34: 2132773-no-tag-please-failtest-5.patch, failed testing. View results

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community

Nice :)

  • catch committed 50e0c6f on 9.1.x
    Issue #2132773 by g-brodiei, killes@www.drop.org, DevJoJodae,...
catch’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 50e0c6f and pushed to 9.1.x. Cherry-picked back to 8.9.x, thanks!

  • catch committed b8eca23 on 9.0.x
    Issue #2132773 by g-brodiei, killes@www.drop.org, DevJoJodae,...

  • catch committed 2eebc47 on 8.9.x
    Issue #2132773 by g-brodiei, killes@www.drop.org, DevJoJodae,...

Status: Fixed » Closed (fixed)

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