Problem/Motivation

Follow up of #2348007: Taxonomy term view needs status filter
In #2348007-30: Taxonomy term view needs status filter catch said

How come we're querying on current language on monolingual sites?

Proposed resolution

Don't filter by language in case the site is monolingual

Remaining tasks

Commit it.

User interface changes

None

API changes

None

CommentFileSizeAuthor
#9 2375879-9.patch8.22 KBjibran
#9 interdiff.txt1.51 KBjibran
#4 2375879-4.patch8.17 KBjibran
#4 interdiff.txt1.88 KBjibran
#1 2375879-1.patch8.08 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2375879-1.patch8.08 KB 

mysql> EXPLAIN SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode
    -> FROM 
    -> node node
    -> INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid
    -> LEFT JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid
    -> WHERE (( (node_field_data.langcode IN  ('en')) ))
    -> ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
    -> LIMIT 11 OFFSET 0;
+----+-------------+-----------------+--------+------------------------------+-----------+---------+-----------------------+------+----------------------------------------------+
| id | select_type | table           | type   | possible_keys                | key       | key_len | ref                   | rows | Extra                                        |
+----+-------------+-----------------+--------+------------------------------+-----------+---------+-----------------------+------+----------------------------------------------+
|  1 | SIMPLE      | node            | index  | PRIMARY                      | node__vid | 5       | NULL                  |    1 | Using index; Using temporary; Using filesort |
|  1 | SIMPLE      | node_field_data | eq_ref | PRIMARY,node_field__langcode | PRIMARY   | 42      | dev_d8.node.nid,const |    1 | Using where; Using index                     |
|  1 | SIMPLE      | taxonomy_index  | ref    | PRIMARY                      | PRIMARY   | 4       | dev_d8.node.nid       |    1 |                                              |
+----+-------------+-----------------+--------+------------------------------+-----------+---------+-----------------------+------+----------------------------------------------+
3 rows in set (0.00 sec)

mysql> EXPLAIN SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid, node_field_data.langcode AS node_field_data_langcode
    -> FROM 
    -> node node
    -> LEFT JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid
    -> ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
    -> LIMIT 11 OFFSET 0;
ERROR 1054 (42S22): Unknown column 'node_field_data.langcode' in 'field list'
mysql> EXPLAIN SELECT taxonomy_index.sticky AS taxonomy_index_sticky, taxonomy_index.created AS taxonomy_index_created, node.nid AS nid
    -> FROM 
    -> node node
    -> LEFT JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid
    -> ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
    -> LIMIT 11 OFFSET 0;
+----+-------------+----------------+-------+---------------+-----------+---------+-----------------+------+----------------------------------------------+
| id | select_type | table          | type  | possible_keys | key       | key_len | ref             | rows | Extra                                        |
+----+-------------+----------------+-------+---------------+-----------+---------+-----------------+------+----------------------------------------------+
|  1 | SIMPLE      | node           | index | NULL          | node__vid | 5       | NULL            |    1 | Using index; Using temporary; Using filesort |
|  1 | SIMPLE      | taxonomy_index | ref   | PRIMARY       | PRIMARY   | 4       | dev_d8.node.nid |    1 |                                              |
+----+-------------+----------------+-------+---------------+-----------+---------+-----------------+------+----------------------------------------------+
2 rows in set (0.01 sec)
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Active » Needs review

Thanks @dawehner for the nice fix. Some minor issues other then this It is RTBC if green.

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -118,6 +119,25 @@ public function testTaxonomyTermView() {
+    debug((string) $view->build_info['query']);

    debug code. 

  2. +++ b/core/modules/views/src/Entity/Render/RendererBase.php
@@ -45,9 +53,12 @@
+  public function __construct(ViewExecutable $view, LanguageManagerInterface $language_manager, EntityTypeInterface $entity_type) {

    This fix is for critical bug so i think it is fine. 

  3. +++ b/core/modules/views/src/Plugin/views/filter/LanguageFilter.php
@@ -28,4 +68,17 @@ public function getValueOptions() {
+    // Don't filter by language in case the site is monolingual, because there

    not multilingual would be better.

Status: Needs review » Needs work

The last submitted patch, 1: 2375879-1.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.88 KB
new 2375879-4.patch8.17 KB

Fixed the test and #2

id select_type table type possible_keys key key_len ref rows Extra
1 SIMPLE taxonomy_index ref PRIMARY,term_node term_node 4 const 5 Using where; Using index
1 SIMPLE node eq_ref PRIMARY PRIMARY 4 d8.taxonomy_index.nid 1 Using index
vijaycs85’s picture

vijaycs85
Location London, UK
commented
Issue tags: +D8MI, +sprint

Overall, looks good to me. +1 to RTBC. setting D8MI tags to get another review from @Gabor.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +language-content

I don't have nearly enough expertise in this area to be able to tell if this is RTBC or not, sorry but it does look good :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Well views changes are fine. I just needed an opinion on D8MI stuff. In #4 I just updated doc and fixed a test so I think after #2, #5 and #6 It is RTBC.
Thank you @vijaycs85 and @Gábor Hojtsy for the review.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -118,6 +119,27 @@ public function testTaxonomyTermView() {
+    $view  = Views::getView('taxonomy_term');

    Additional space. 

  2. +++ b/core/modules/views/src/Entity/Render/RendererBase.php
@@ -45,9 +53,12 @@
    * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    *   The entity type.
+   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
+   *   The language manager.
...
-  public function __construct(ViewExecutable $view, EntityTypeInterface $entity_type) {
+  public function __construct(ViewExecutable $view, LanguageManagerInterface $language_manager, EntityTypeInterface $entity_type) {

    The docblock has the arguments in the wrong order.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.51 KB
new 2375879-9.patch8.22 KB

Fixed #8.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

@TODO

:)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Done

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a major task that will improve performance and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 56e29e0 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
index 8ae1a76..fbe065a 100644
--- a/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -120,7 +120,7 @@ public function testTaxonomyTermView() {
     $this->assertNoText($original_title);
     $this->assertText($translated_title);
 
-    // Disable language module and ensure that the language is not part of the
+    // Uninstall language module and ensure that the language is not part of the
     // query anymore.
     // @see \Drupal\views\Plugin\views\filter\LanguageFilter::query()
     \Drupal::moduleHandler()->uninstall(['content_translation', 'language']);

Fixed on commit.

  • alexpott committed 56e29e0 on 8.0.x 
    Issue #2375879 by jibran, dawehner: Don't filter languages in case it is...
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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