Issue:

Parent term filter is unavailable in Beta9.

Reason:

Parent terms should join on "taxonomy_term_field_data" instead of "taxonomy_term_data" since the base_table for taxonomy terms have changed.

Fix:

In "core/modules/taxonomy/src/TermViewsData.php"

-      'taxonomy_term_data'
+      'taxonomy_term_field_data'
CommentFileSizeAuthor
#20 taxonomy_index_and-2459777-20.patch12.51 KBjibran
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,489 pass(es). View
#20 interdiff.txt1010 bytesjibran
#18 taxonomy_index_and-2459777-18.patch12.6 KBjibran
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,463 pass(es), 0 fail(s), and 1 exception(s). View
#18 interdiff.txt792 bytesjibran
#13 taxonomy_index_and-2459777-13-test-only.patch10.41 KBjibran
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,993 pass(es), 5 fail(s), and 6 exception(s). View
#13 taxonomy_index_and-2459777-13.patch13.44 KBjibran
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,009 pass(es). View
#10 2456713-fix-only.patch2.33 KBjibran
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,943 pass(es). View
#1 parent_filter_unavailable-2459777-1.patch556 bytesJaesin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,097 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Jaesin’s picture

Status: Active » Needs review
FileSize
556 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,097 pass(es). View

Fix applies to 8.0.x (#c7fd064).

tim.plunkett’s picture

Version: 8.0.0-beta9 » 8.0.x-dev
Issue tags: -#taxonomy, -#views +VDC, +Needs tests

Thanks! That looks sane. We should probably have some automated test coverage of this.

jibran’s picture

Title: Parent term filter is unavailable in Beta9 » Parent term filter is missing
Priority: Normal » Major
Status: Needs review » Needs work
John Cook’s picture

Status: Needs work » Reviewed & tested by the community

Tested patch #1 and fixes error.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Still needs a test. @John Cook see http://drupal.org/simpletest for a start on using SimpleTest. | Automated testing in Drupal: http://bit.ly/mwds-testing-video | Writing tests for core: https://drupal.org/contributor-tasks/write-tests

jibran’s picture

Yeah but we need tests to make sure it that it won't happen again.

jibran’s picture

Yeah but we need tests to make sure it that it won't happen again.

jibran’s picture

Working on the test and fix is incomplete

diff --git a/core/modules/taxonomy/src/TermViewsData.php b/core/modules/taxonomy/src/TermViewsData.php
index d101f84..61a3f65 100644
--- a/core/modules/taxonomy/src/TermViewsData.php
+++ b/core/modules/taxonomy/src/TermViewsData.php
@@ -255,7 +255,7 @@ public function getViewsData() {
         'left_field' => 'tid',
         'field' => 'parent',
       ),
-      'taxonomy_term_data' => array(
+      'taxonomy_term_field_data' => array(
         // Link directly to taxonomy_term_data via tid.
         'left_field' => 'tid',
         'field' => 'tid',
@@ -266,7 +266,7 @@ public function getViewsData() {
       'title' => t('Parent term'),
       'help' => t('The parent term of the term. This can produce duplicate entries if you are using a vocabulary that allows multiple parents.'),
       'relationship' => array(
-        'base' => 'taxonomy_term_data',
+        'base' => 'taxonomy_term_field_data',
         'field' => 'parent',
         'label' => t('Parent'),
         'id' => 'standard',

Also #2456713: Custom taxonomy field views handler needs to be replaced with generic Field API handler is related I thought we can move this fix in that issue but that is separate problem.

TR’s picture

Status: Needs work » Needs review
FileSize
1004 bytes

Previous patch only fixes this at one spot, there are actually two lines that need to be changed to fix the parent term.

To reproduce this bug, create a new Taxonomy term view, try to add a filter (or a contextual filter, or a relationship) on the parent term and you'll see that's not one of your choices anymore. Try in D7, and it's there. Initial patch didn't fix the relationship.

This is a rather small fix for a mistake in a previous commit, what do you suggest as far as tests? The original issue #2429447: Use data table as views base table, if available. that broke this wasn't required to have any tests - it seems to me that it should be easier to fix things and less easy to break things...

Adding a test to check just the parent term seems a little weak - what about the other thousand terms defined in core?

jibran’s picture

Title: Parent term filter is missing » {taxonomy_index} and {taxonomy_term_hierarchy} doesn't join taxonomy data table in views
FileSize
2.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,943 pass(es). View

Found one more issue while scanning TermViewsData. Here is fix only patch comments/suggestions welcome.

jibran’s picture

This is a rather small fix for a mistake in a previous commit

If we'd have test back then we wouldn't have introduced this bug on the first place.

jhodgdon’s picture

Right, and when we find bugs and fix them in issues, our general requirement is also to add a test so they do not come back again.

jibran’s picture

Assigned: jibran » Unassigned
Issue tags: -Needs tests
FileSize
13.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,009 pass(es). View
10.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,993 pass(es), 5 fail(s), and 6 exception(s). View

Added test for views data generation and checked both relationships for a view. Test only patch is an interdiff.

jibran’s picture

+++ b/core/modules/taxonomy/src/TermViewsData.php
@@ -248,6 +248,7 @@ public function getViewsData() {
+    $data['taxonomy_term_hierarchy']['table']['provider']  = 'taxonomy';

It is views by default which is not true. Is it worth it to add?

Status: Needs review » Needs work

The last submitted patch, 13: taxonomy_index_and-2459777-13-test-only.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

/me likes Drupal\Core\Database\DatabaseExceptionWrapper: Exception in test_taxonomy_term_relationship[test_taxonomy_term_relationship]: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') subquery' at line 2: SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {taxonomy_term_field_data} taxonomy_term_field_data LEFT JOIN {node} node_taxonomy_index ON nid = node_taxonomy_index.nid LEFT JOIN {taxonomy_term_data} taxonomy_term_data_taxonomy_term_hierarchy ON parent = taxonomy_term_data_taxonomy_term_hierarchy.) subquery; Array ( ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1456 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/Plugin/views/query/Sql.php). Drupal\views\Plugin\views\query\Sql->execute(Object) Drupal\views\ViewExecutable->execute() Drupal\views\Tests\ViewTestBase->executeView(Object) Drupal\taxonomy\Tests\Views\TaxonomyRelationshipTest->testTaxonomyRelationships() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('1363', 'Drupal\taxonomy\Tests\Views\TaxonomyRelationshipTest')

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +regression

Just hit this bug. Patch looks great and test does as well.

Nitpick can either be ignored or hopefully fixed on commit, so marking RTBC.

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyRelationshipTest.php
@@ -0,0 +1,112 @@
+    $this->terms[] =$this->term1;
+    $this->terms[] =$this->term2;

Nitpick: There should be a space after the "=".

jibran’s picture

FileSize
792 bytes
12.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,463 pass(es), 0 fail(s), and 1 exception(s). View

Reroll and fixed #17

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: taxonomy_index_and-2459777-18.patch, failed testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1010 bytes
12.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,489 pass(es). View

There is nothing to review here so back to RTBC.

tstoeckler’s picture

Yes, thanks, RTBC++

  • xjm committed bfdcc65 on 8.0.x
    Issue #2459777 by jibran, Jaesin, tstoeckler, TR: {taxonomy_index} and {...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Nice find! Thanks for the fix and test. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.

+++ b/core/modules/taxonomy/src/TermViewsData.php
@@ -169,8 +169,8 @@ public function getViewsData() {
-    // @todo This stuff needs to move to a node field since really it's all about
-    //   nodes.
+    // @todo This stuff needs to move to a node field since really it's all
+    //   about nodes.

Note: In the future, please keep coding standards cleanups like rewrapping comments separate from bugfixes, since it makes patches harder to review, adds unnecessary merge conflicts, etc. Reference: http://webchick.net/please-stop-eating-baby-kittens

Status: Fixed » Closed (fixed)

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