Problem/Motivation

This issue has been promoted to critical because it also resolves #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors (in addition to being a beta target and part of the scope of the VDC initiative).

Similar to #1806334: Replace the node listing at /node with a view. The default taxonomy term listing page is inflexible, and Views has long shipped with a replacement.

Proposed resolution

  • Remove the custom code and use the view.
  • Remove the depth argument from the default view for a more performant page.

Queries before conversion

SELECT COUNT(t.nid) AS expression FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0)
SELECT t.nid AS nid, t.tid AS tid, t.sticky AS sticky, t.created AS created FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0) ORDER BY t.sticky DESC, t.created DESC LIMIT 10 OFFSET 0

Query after conversion

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 taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid WHERE (( (taxonomy_index.tid = :taxonomy_index_tid) )) ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC LIMIT 11 OFFSET 0

Per @dawehner, the node JOIN in this query is unnecessary, and an as-yet unfiled followup issue for #1740492: Implement a default entity views data handler can enable views to rely on the field data table without needing the base table when possible.

Remaining tasks

User interface changes

The feed icon is no longer visible on term pages to access the taxonomy term feeds. (It was already broken in D8 HEAD for awhile, linking to the wrong path.) This will be resolved by:

API changes

  • taxonomy/term/% pages are replaced with a view.
  • The following callbacks/page builders are removed:
    • taxonomy_term_page()
    • taxonomy_term_feed()
    • node_feed()
    • TaxonomyController::termPage()
    • TaxonomyController::termFeed()

Original report by @tim.plunkett

CommentFileSizeAuthor
#216 vdc-1857256-216.patch40.23 KBdawehner
#216 interdiff.txt1.01 KBdawehner
#209 vdc-1857256-206.patch40.17 KBGábor Hojtsy
#206 interdiff.txt1.58 KBdawehner
#206 vdc-1857256-206.patch40.17 KBdawehner
#203 vdc-1857256-203.patch40.72 KBdawehner
#203 interdiff.txt759 bytesdawehner
#199 interdiff-198-from-178.txt7.24 KBxjm
#199 taxonomy-views-1857256-198.patch42.5 KBxjm
#197 interdiff.txt4.27 KBjibran
#197 taxonomy_term-1857256-197.patch40.41 KBjibran
#194 interdiff.txt5.18 KBdawehner
#194 vdc-1857256-194.patch36.13 KBdawehner
#186 new_explains.png15.45 KBxjm
#186 old_explains.png17.32 KBxjm
#184 taxonomy-views-1857256-183.patch32.37 KBxjm
#178 interdiff.txt7.24 KBdawehner
#178 taxonomy_term_view-1857256-178.patch38.74 KBdawehner
#174 interdiff.txt1.97 KBdawehner
#174 taxonomy_term-1857256-174.patch32.35 KBdawehner
#166 taxonomy_term-1857256-166.patch36.4 KBjibran
#165 2324809-test-only.patch3.94 KBjibran
#165 taxonomy_term-1857256-164.patch33.15 KBjibran
#161 interdiff.txt754 bytesdawehner
#161 taxonomy_term-1857256-161.patch29.21 KBdawehner
#158 interdiff.txt1.12 KBdawehner
#158 1857256-views-158.patch28.68 KBdawehner
#153 convert_the_taxonomy-views-1857256-153.patch28.76 KBhussainweb
#149 vdc-1857256-149.patch29.49 KBParisLiakos
#143 interdiff.txt2.54 KBParisLiakos
#143 vdc-1857256-143.patch30.01 KBParisLiakos
#141 interdiff.txt905 bytesParisLiakos
#141 vdc-1857256-141.patch28.76 KBParisLiakos
#133 interdiff.txt554 bytesdawehner
#133 vdc-1857256-133.patch28.33 KBdawehner
#131 vdc-1857256-131.patch26.41 KBdawehner
#131 interdiff.txt2.7 KBdawehner
#128 interdiff.txt2.66 KBdawehner
#128 vdc-1857256-128.patch24.91 KBdawehner
#126 vdc-1857256-126.patch23.96 KBoadaeh
#121 interdiff-119-121.txt731 bytesxjm
#121 vdc-1857256-121.patch24 KBxjm
interdiff-119-120.txt730 bytesxjm
vdc-1857256-120.patch24 KBxjm
#119 interdiff-114-119.txt1.07 KBxjm
#119 vdc-1857256-119.patch24 KBxjm
#114 interdiff.txt896 bytesdawehner
#114 vdc-1857256-114.patch23.31 KBdawehner
#113 interdiff.txt5.39 KBdawehner
#113 vdc-1857256-113.patch23.3 KBdawehner
#111 interdiff.txt5.83 KBdawehner
#111 vdc-1857256-111.patch18.08 KBdawehner
#109 rss_icon.png40.17 KBxjm
#108 without_views.png8.48 KBxjm
#108 its_a_view.png33.84 KBxjm
#108 view_configuration.png112.79 KBxjm
#107 interdiff.txt670 bytestim.plunkett
#107 taxonomy_term-1857256-107.patch15.14 KBtim.plunkett
#101 interdiff.txt1.88 KBdawehner
#101 taxonomy_term-1857256-101.patch15.13 KBdawehner
#98 interdiff.txt560 bytesdawehner
#98 taxonomy_term-1857256-98.patch15.31 KBdawehner
#96 interdiff.txt5.99 KBdawehner
#96 taxonomy-1857256-96.patch15.31 KBdawehner
#93 vdc-1857256-93.patch9.86 KBdawehner
#81 vdc-1857256-81.patch20.15 KBhussainweb
#79 vdc-1857256-79.patch32.49 KBhussainweb
#76 vdc-1857256-76.patch41.65 KBrhabbachi
#73 vdc-1857256-73.patch36.06 KBInternetDevels
#64 interdiff.txt11.97 KBdawehner
#62 1857256-taxonomy-views-62.patch20.83 KBvijaycs85
#59 vdc-1857256-59.patch30.43 KBpcambra
#59 interdiff.txt8.6 KBpcambra
#57 vdc-1857256-57.patch26.22 KBpcambra
#47 term-1857256-47.patch26.84 KBekes
#44 term-1857256-43.patch20.11 KBekes
#39 interdiff.txt520 bytestim.plunkett
#39 term-1857256-39.patch3.17 KBtim.plunkett
#37 interdiff.txt2.2 KBtim.plunkett
#37 term-1857256-37.patch2.93 KBtim.plunkett
#33 taxonomy-view-1857256-33.patch1.32 KBrdrh555
#19 taxonomy-term-after.png41.14 KBParisLiakos
#19 taxonomy-term-before.png39.96 KBParisLiakos
#18 drupal-1857256-18.patch19.22 KBdawehner
#15 drupal-1857256-15.patch19.33 KBtstoeckler
#14 drupal-1857256-14.patch18.4 KBdawehner
#9 interdiff.txt3.05 KBdawehner
#9 drupal-1857256-9.patch17.93 KBdawehner
#7 drupal-1857256-7.patch18.1 KBdawehner
#4 interdiff.txt2.53 KBdawehner
#4 drupal-1857256-4.patch13.95 KBdawehner
#3 interdiff.txt3.86 KBtim.plunkett
#3 vdc-1857256-2.patch14.47 KBtim.plunkett
#1 vdc-1857256-1.patch10.82 KBtim.plunkett
#88 vdc-1857256-88.patch16.68 KBhussainweb
#64 vdc-1857256.patch20.21 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
10.82 KB

See attached. 27 insertions(+), 125 deletions(-) is nice.

Status: Needs review » Needs work

The last submitted patch, vdc-1857256-1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.86 KB
14.47 KB

Well taxonomy_term_page() had some really hacky code for breadcrumbs, I've done an equally hacky job at porting that.

dawehner’s picture

FileSize
2.53 KB
13.95 KB

You know my friend, learn the power of views.

Status: Needs review » Needs work

The last submitted patch, drupal-1857256-4.patch, failed testing.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.1 KB

Summary of the test failures:

  • Drupal\taxonomy\Tests\TermTest: Relies on the area handling
  • Drupal\taxonomy\Tests\RssTest: Relies on the path taxonomy/term/%term/feed but views uses %term/%modifier/feed. I think we should just support the feature provided by core, so let's get rid of the modifier for now? People could still use it.
  • Drupal\system\Tests\Theme\EntityFilteringThemeTest: Relies on the area handling
  • Drupal\system\Tests\Module\EnableDisableTest: Relies on some assumption that config will be always imported, see Node issue
  • Drupal\rdf\Tests\MappingDefinitionTest: Relies on the area handling
  • Drupal\path\Tests\PathTaxonomyTermTest: Relies on the area handling
  • Drupal\system\Tests\Menu\BreadcrumbTest: I seriously don't like you! (this is all about breadcrumbs ... )

Status: Needs review » Needs work

The last submitted patch, drupal-1857256-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
17.93 KB

Got the breadcrumbs running, so it's just a matter of getting the entity area rendering and configure it here.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, drupal-1857256-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#9: drupal-1857256-9.patch queued for re-testing.

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

The last submitted patch, drupal-1857256-9.patch, failed testing.

xjm’s picture

podarok’s picture

Issue summary: View changes

add meta

dawehner’s picture

FileSize
18.4 KB

Against, this issue is currently blocked on #1857376: Provide an area handler that renders an entity

That's just a rerole.

tstoeckler’s picture

FileSize
19.33 KB

This is another re-roll. Haven't tried it out yet.

dawehner’s picture

Status: Needs work » Needs review

Set to needs review to see what errors are missing.

Status: Needs review » Needs work

The last submitted patch, drupal-1857256-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.22 KB

Let's use the new area handler now.

ParisLiakos’s picture

Just tried this, seems though that we need to get rid of title in the header view area of the term. See screenshots

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1689,7 +1689,9 @@ public function getBreadcrumb($set = FALSE) {
+          // Just add the frontpage (so the first item of the breadcrumbs).
+          $existing_breadcrumb = drupal_get_breadcrumb();
+          $breadcrumb = array_merge(array($existing_breadcrumb[0]), $breadcrumb);

really minor: the comment needs some refining..

tstoeckler’s picture

I suppose we could add a setting to the area handler that suppresses the title. It seems that not only taxonomy terms could profit from that. Maybe we should only show that setting for the 'full' view mode? Hmm...

dawehner’s picture

Having a $page variable seems to be a concept which exists on different kind of entities (both node and taxonomy have it).
Now that taxonomy/term is a view, taxonomy_term_is_page doesn't seem to make any sense anymore, as you really want to have full control over it.

I'm wondering whether there is a plan to make the label configurable on the display bundle settings? Wouldn't this basically allow the site builder to create a
display format, which don't contain the label?

ParisLiakos’s picture

Status: Needs review » Needs work

That would require terms going NG first.
why dont we make taxonomy_term_is_page() work with views for now?

dawehner’s picture

Well, as you can't use it with menu_get_object() anymore, it seems to be that you need to use arg() or get the
argument from the current request object. Does someone has a better idea?

ParisLiakos’s picture

how about using menu_base_path from Term Enity plugin annotation?

dawehner’s picture

Well, yeah we could remove %taxonomy_term from this string and then break again, once people realize that this feature doesn't work together with route items, mh.

xjm’s picture

dawehner’s picture

andypost’s picture

By using taxonomy_menu module for D7 I got a lot of problems with performance caused by the way views arguments.
So for each menu router overriden by view I got access check that initializes view to check access

dawehner’s picture

Well, what should do views, not validate the stuff? In Drupal 8 once #1800998: Use route system instead of hook_menu() in Views is in, we might should see
whether we can move validation to the route level as well, so we don't need an actual view instance.

ParisLiakos’s picture

Title: Convert the taxonomy listing at /taxonomy/term/%term to Views » Convert the taxonomy listing and feed at /taxonomy/term/%term to Views

retitling to include the feed, i just closed the two conversion issues for this one

trevjs’s picture

Assigned: Unassigned » trevjs
rdrh555’s picture

Assigned: trevjs » rdrh555
rdrh555’s picture

Enable the Taxonomy term view and modified the Page display at Path: /taxonomy/term/% .
Original view was using mini-pager and showed "Page 1" at the bottom when there was only one item to show. Changed the pager to "full" and tested. Pager now displays numbers and there is no pager on a page with 1 item.

dawehner’s picture

Thanks for testing it.

Enable the Taxonomy term view and modified the Page display at Path: /taxonomy/term/% .
Original view was using mini-pager and showed "Page 1" at the bottom when there was only one item to show. Changed the pager to "full" and tested. Pager now displays numbers and there is no pager on a page with 1 item.

There has been an issue for that: #1998330: Minipager is broken on page size == 1 due to ceil(PHP_MAX_INT / 1)

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -VDC

#18: drupal-1857256-18.patch queued for re-testing.

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

The last submitted patch, taxonomy-view-1857256-33.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
2.93 KB

Status: Needs review » Needs work

The last submitted patch, term-1857256-37.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
520 bytes
3.17 KB

Missed one change.

Status: Needs review » Needs work

The last submitted patch, term-1857256-39.patch, failed testing.

tim.plunkett’s picture

It's getting the parent menu item's link title instead of the term. I feel like we fixed this before...

jibran’s picture

klonos’s picture

ekes’s picture

FileSize
20.11 KB

Combined patch from changes made (and still required, or otherwise not already applied) in #18 corrected to apply to head, with #39.

Additional change to account for legacy breadcrumb system no longer having any content by default. Later #2026073: Convert drupal_set_breadcrumb/ViewExecutable::getBreadcrumb() to breadcrumb builder service in ViewExecutable could remove any dependency on the legacy system.

ekes’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, term-1857256-43.patch, failed testing.

ekes’s picture

Status: Needs work » Needs review
FileSize
26.84 KB

Missing the moved file from the patch. Try again - Sorry.

Status: Needs review » Needs work

The last submitted patch, term-1857256-47.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/taxonomy/config/views.view.taxonomy_term.ymlundefined
@@ -0,0 +1,251 @@
+          perm: 'access content'

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -248,20 +248,6 @@ function taxonomy_menu() {
-    'access callback' => 'entity_page_access',
-    'access arguments' => array(2, 'view'),

Pretty sure this is causing fails.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/IndexTid.phpundefined
@@ -40,6 +40,7 @@ public function setBreadcrumb(&$breadcrumb) {
+    module_load_include('inc', 'taxonomy', 'taxonomy.views');

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument/IndexTidDepth.phpundefined
@@ -74,6 +62,7 @@ public function setBreadcrumb(&$breadcrumb) {
+    module_load_include('inc', 'taxonomy', 'taxonomy.views');

Is this really needed?! :(

ekes’s picture

There are two reasons the the tests are failing:-

First. The present hook_menu() item that is being replaced does an entity access view check for the access callback. The view adds an access content permission check; but doesn't have a entity access check. Two solutions suggested:

Second. Even with access to the page. The test requires the title (parent item) to be the name of the term. The view is now setting the parent item rather than the hook_menu item. This doesn't have replacement for the entity (it is presently hard coded to 'Taxonomy term'). Suggested options: set page title from the hook_menu for taxonomy/term/%taxonomy_term/edit; or if possible enable 'parent item' to be able to do entity %1 replacements.

dawehner’s picture

Assigned: rdrh555 » Unassigned

I would really go with the drupal_set_title() idea for now. This is the way to make progress

xjm’s picture

dawehner’s picture

jibran’s picture

Issue summary: View changes

Correct meta issue.

dawehner’s picture

klonos’s picture

Issue summary: View changes
Parent issue: » #1823450: [Meta] Convert core listings to Views
pcambra’s picture

Status: Needs work » Needs review
FileSize
26.22 KB

Here's a re-roll of [#47] to see what's the shape of this after some months of changes

Status: Needs review » Needs work

The last submitted patch, 57: vdc-1857256-57.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
8.6 KB
30.43 KB

After discussing with @dawehner on IRC, we're reverting the changes that remove the fallback listings as it seems is an easier path. Taxonomy routing elements (for page at least) are too coupled to just remove them like that.

The possible paths seems to be keep the listing and solve the comments in #50 to implement the breadcrumb or allow Views UI to have a new setting to set the route name (if we go this way we'd need a separate issue).

New re-roll of the patch fixing the above stuff and some other minor things.

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, 59: vdc-1857256-59.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.83 KB

Status: Needs review » Needs work

The last submitted patch, 62: 1857256-taxonomy-views-62.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.21 KB
11.97 KB

A couple of bugs in Entity and views:

  • Term view config file changed a lot of values which got into core in the meantime
  • The path plugin base don't deal perfectly with optional parameters with similar names as other parameters (added a unit test)
  • The entity classes used a non url generator based approach for uri() which is wrong for several reasons.

The last submitted patch, 64: vdc-1857256.patch, failed testing.

vijaycs85’s picture

64: vdc-1857256.patch queued for re-testing.

The last submitted patch, 64: vdc-1857256.patch, failed testing.

dawehner’s picture

64: vdc-1857256.patch queued for re-testing.

The last submitted patch, 64: vdc-1857256.patch, failed testing.

The last submitted patch, 64: vdc-1857256.patch, failed testing.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +DrupalWorkParty

Tagging for reroll.

rhabbachi’s picture

Assigned: Unassigned » rhabbachi
Issue tags: +TUNIS_2014_JANUARY
InternetDevels’s picture

Status: Needs work » Needs review
FileSize
36.06 KB

Unfortunately for me I've created a patch yesterday, but haven't posted it because interdiff looked suspicious. I've seen that you've tagged this issue for your sprint, but decided to post my patch anyway, maybe it can be useful for you.

dawehner’s picture

Thank you for rerolling the patch but please don't be dissapointed if it gets red.

Status: Needs review » Needs work

The last submitted patch, 73: vdc-1857256-73.patch, failed testing.

rhabbachi’s picture

FileSize
41.65 KB
rhabbachi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 76: vdc-1857256-76.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
32.49 KB

I rebased this from #64. I checked the diff's carefully after InternetDevel's comment in #73. I hope the reroll works.

Status: Needs review » Needs work

The last submitted patch, 79: vdc-1857256-79.patch, failed testing.

hussainweb’s picture

Attaching a bugfix. The patch used $entity_info as an array but it is supposed to be an object (it might have changed later). I did manual tests and the term pages loaded fine. I see that it is using views from the generated markup.

I also generated a diff with -M switch to detect rename of the view YAML file. Hence, the patch is much smaller.

hussainweb’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -173,7 +173,7 @@ public function uri($rel = 'canonical') {
-      $uri['path'] = str_replace(array_keys($replacements), array_values($replacements), $template);
+      $uri['path'] = \Drupal::urlGenerator()->getPathFromRoute($link_templates[$rel], $replacements);

@@ -243,12 +243,16 @@ protected function linkTemplates() {
-        '{entityType}' => $this->entityType(),
-        '{bundle}' => $this->bundle(),
-        '{id}' => $this->id(),
-        '{uuid}' => $this->uuid(),
-        '{' . $this->entityType() . '}' => $this->id(),
+        'entityType' => $this->entityType(),
+        'bundle' => $this->bundle(),
+        'id' => $this->id(),
+        'uuid' => $this->uuid(),
+        $this->entityType() => $this->id(),
       );
+      $entity_info = \Drupal::entityManager()->getDefinition($this->entityType());
+      if (isset($entity_info->bundle_entity_type)) {
+        $this->uriPlaceholderReplacements[$entity_info->bundle_entity_type] = $this->bundle();
+      }

Why is this part of this patch? This looks similar to the changes in #2167641: EntityInterface::uri() should use route name and not path, but only halfway.

hussainweb’s picture

It was a part of the patch in #64. dawehner mentioned there were some bugs in Entity and Views implementation and I just assumed they were fixes, though, like you said, should be part of a different issue. Should we block this until that patch goes in?

Status: Needs review » Needs work

The last submitted patch, 81: vdc-1857256-81.patch, failed testing.

hussainweb’s picture

I see the failures are probably related to the changes mentioned in #83. I think we will have to wait for #2167641: EntityInterface::uri() should use route name and not path here?

hussainweb’s picture

The reason for the failure is this:

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -173,7 +173,7 @@ public function uri($rel = 'canonical') {
-      $uri['path'] = str_replace(array_keys($replacements), array_values($replacements), $template);
+      $uri['path'] = \Drupal::urlGenerator()->getPathFromRoute($link_templates[$rel], $replacements);

For $rel == 'admin-form', the route is /admin/structure/types/manage/{node_type}. And {node_type} is not available down the line in Symfony\Component\Routing\Generator\UrlGenerator::doGenerate.

I don't think the issue should be tackled here. Should we wait until this is fixed in #2167641: EntityInterface::uri() should use route name and not path? I will try later without these parts if I get a chance and see what is the bug.

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.68 KB

Status: Needs review » Needs work

The last submitted patch, 88: vdc-1857256-88.patch, failed testing.

catch’s picture

Priority: Normal » Critical

#2102485: Remove drupal_set_title in taxonomy module controllers is currently postponed on this, and that in turn blocks #1830588: [META] remove drupal_set_title() and drupal_get_title(). I don't think there's a real dependency, but since there's active work here and not on the postponed issue, bumping to critical. If this turns out to be difficult to get done, we should consider unpostponing the other issue though instead (since it's the very last drupal_set_title() in core).

ianthomas_uk’s picture

Assigned: rhabbachi » Unassigned
Priority: Critical » Normal

Back to normal, as #2102485: Remove drupal_set_title in taxonomy module controllers has actually already been fixed.

I had a look over the patch and the failing tests though:
- DefaultConfigTest failure is easy, just needs '1' changed to true on line 5 of views.view.taxonomy_term.yml.
- PathTaxonomyTermTest is failing because the description isn't showing on the view term page (nor is the rss link).
- We've moving the contents of taxonomy_term_page and taxonomy_term_feed into TaxonomyController. Wouldn't it be better to do that in a separate issue, so that any logical changes to the contents of those functions are easier to review?
- TaxonomyController::termPage() reintroduces a call to drupal_add_title. It's probably using an older version of the function and changes got lost in re-rolls.
- Why do we even have TaxonomyController::termPage() and termFeed()? Isn't the point of this issue to remove them?

Unassigning from rhabbachi as other people have been working on patches, but feel free to re-assign if you're actively working on this.

sun’s picture

Would be great to move forward here, since this conversion allows us to remove a whole bunch of legacy procedural API functions throughout core (taxonomy_term_feed()node_feed()format_rss_*(), etc.)

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.86 KB

Let's see how much is broken.

Status: Needs review » Needs work

The last submitted patch, 93: vdc-1857256-93.patch, failed testing.

xjm’s picture

Issue tags: +beta target
dawehner’s picture

Status: Needs work » Needs review
FileSize
15.31 KB
5.99 KB

We are getting there.

Status: Needs review » Needs work

The last submitted patch, 96: taxonomy-1857256-96.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.31 KB
560 bytes
xjm’s picture

Priority: Normal » Major

Status: Needs review » Needs work

The last submitted patch, 98: taxonomy_term-1857256-98.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.13 KB
1.88 KB

Now we just need the schema issue.

Status: Needs review » Needs work

The last submitted patch, 101: taxonomy_term-1857256-101.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Postponed

Let's postpone this as the 4 failures are fixed by #2246557: Missing schema for entity area plugin

jibran’s picture

jibran’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 101: taxonomy_term-1857256-101.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.14 KB
670 bytes

Yay for schemas!

xjm’s picture

FileSize
112.79 KB
33.84 KB
8.48 KB

Green patch, happy happy.

Did some manual testing.

All the contextual links and such work fine.

When Views is uninstalled, it just falls back to just rendering the entity with no listing:

Next a code review.

xjm’s picture

FileSize
40.17 KB

Mmm, is "term with depth" the right contextual filter? I don't think HEAD supports a depth argument, does it? and this would also change the feed paths, right?

Speaking of, we also need to add the feed icon to the view.

Not that that link works in HEAD... you get

The requested page "/taxonomy/term/taxonomy/term/1/feed" could not be found.

...So good thing that we're removing it. And sounds like we also need to add some test coverage for the feed display.

xjm’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TaxonomyController.php
@@ -71,11 +75,23 @@ public function termTitle(TermInterface $taxonomy_term) {
-   * @todo Remove taxonomy_term_feed().
+   * Generate the content feed for a taxonomy term.
+   *
+   * @param \Drupal\taxonomy\TermInterface $term
+   *   The taxonomy term entity.
+   *
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   A response object.
    */
   public function termFeed(TermInterface $taxonomy_term) {
-    module_load_include('pages.inc', 'taxonomy');
-    return taxonomy_term_feed($taxonomy_term);
+    $channel['link'] = $this->url('taxonomy.term_page', array('taxonomy_term' => $taxonomy_term->id()), array('absolute' => TRUE));
+    $channel['title'] = \Drupal::config('system.site')->get('name') . ' - ' . $taxonomy_term->label();
+    // Only display the description if we have a single term, to avoid clutter and confusion.
+    // HTML will be removed from feed description.
+    $channel['description'] = check_markup($taxonomy_term->description->value, $taxonomy_term->description->format, '', TRUE);
+    $nids = taxonomy_select_nodes($taxonomy_term->id(), FALSE, \Drupal::config('system.rss')->get('items.limit'));
+
+    return node_feed($nids, $channel);
   }
 

Oh... hm. I'd be in favor of removing this and requiring views for the feed.

Edit, re-edited: There's no fallback provided for the node feed without Views enabled in HEAD, which I think is good. So I wonder if we can remove node_feed() after we convert this?

dawehner’s picture

FileSize
18.08 KB
5.83 KB

schnaps-number!

Mmm, is "term with depth" the right contextual filter? I don't think HEAD supports a depth argument, does it? and this would also change the feed paths, right?

In Drupal 6 core had that feature, and got removed in D7 with the promise that views implements it.
That is the reason why the d7 view which is basically still the same as in d8 has that feature. I really think we should keep it.

ParisLiakos’s picture

+++ /dev/null
@@ -1,81 +0,0 @@
-function taxonomy_term_feed(Term $term) {
...
-  return node_feed($nids, $channel);

lets also get rid of node_feed and resolve that todo there? (EDIT: actually nvm, cause that means we would have to also remove format_rss_channel and probably more..so lets do it in a followup)

Also, shouldnt the feed be attached to the page display, so that we have the feed icon in the page?

dawehner’s picture

dawehner’s picture

FileSize
23.31 KB
896 bytes

Also, shouldnt the feed be attached to the page display, so that we have the feed icon in the page?

We can't do at the moment, #2251121: Support to add a feed icon on pages with parameters is a follow up.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks for opening those follow ups!

ParisLiakos’s picture

xjm’s picture

Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

+1 on the RTBC and the followups.

This needs a change record; I'm working on it.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Alrighty, change record is set. :) Added to https://drupal.org/node/2084727.

xjm’s picture

Just a couple little comment fixes.

xjm’s picture

FileSize
24 KB
731 bytes

Whoops, fixed the wrong comment. Being a little naughty and left in the other fixed comment as well so that they're the same. :P ...And reuploading the right thing.

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

The depth argument was taken out of the taxonomy/term view in core at least partly due to performance. We shouldn't support that in the default view for the same reason. It's still 'provided by views' either way.

Could someone show the raw query or an EXPLAIN before and after on the term pages to ensure there's no regression in the query?

xjm’s picture

Issue tags: +needs profiling
oadaeh’s picture

FileSize
23.96 KB

This is simply a re-roll of @xjm's patch to account for the recent core changes. None of the issues raised have been addressed. All tests pass locally.

ParisLiakos’s picture

Status: Needs work » Needs review

OK, i created 3 terms

test
-test1
--test2

Visiting the test2 term page as user 1:

Before:
SELECT t.nid AS nid, t.tid AS tid, t.sticky AS sticky, t.created AS created FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0) ORDER BY t.sticky DESC, t.created DESC LIMIT 10 OFFSET 0

After:
SELECT node_field_data.sticky AS node_field_data_sticky, node_field_data.created AS node_field_data_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 WHERE (( (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (node.nid IN (SELECT tn.nid AS nid FROM taxonomy_index tn WHERE ( (tn.tid = :db_condition_placeholder_0_0) ))) )) ORDER BY node_field_data_sticky DESC, node_field_data_created DESC LIMIT 11 OFFSET 0

I guess the real difference here is the subquery. Also you can see that views cares for sticky field, but you can easily remove it.

as far as the depth is concerned, it is the same.
it only accounts for depth if you add the path argument for that. eg taxonomy/term/1/2
where the query looks like:

SELECT node_field_data.sticky AS node_field_data_sticky, node_field_data.created AS node_field_data_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
WHERE (( (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (node.nid IN  (SELECT tn.nid AS nid
FROM 
{taxonomy_index} tn
LEFT OUTER JOIN {taxonomy_term_hierarchy} th ON th.tid = tn.tid
LEFT OUTER JOIN {taxonomy_term_hierarchy} th1 ON th.parent = th1.tid
LEFT OUTER JOIN {taxonomy_term_hierarchy} th2 ON th1.parent = th2.tid
WHERE ( (tn.tid = '1') OR (th1.tid = '1') OR (th2.tid = '1') ))) ))
ORDER BY node_field_data_sticky DESC, node_field_data_created DESC
LIMIT 11 OFFSET 0

i guess back to catch?

dawehner’s picture

FileSize
24.91 KB
2.66 KB

Just changed the file by hand.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

bot can object if it wants to

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 128: vdc-1857256-128.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
26.41 KB

Just changed the file by hand.

HAHA, what a fail!

Status: Needs review » Needs work

The last submitted patch, 131: vdc-1857256-131.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.33 KB
554 bytes

It is SO funny to see that even the config team don't give a fuck about config schema.

Status: Needs review » Needs work

The last submitted patch, 133: vdc-1857256-133.patch, failed testing.

dawehner’s picture

133: vdc-1857256-133.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work

That subquery doesn't look encouraging. If we can demonstrate that's not a problem (via a large dataset + EXPLAIN) then fine, but more likely we'll need to refactor to remove that I think.

dawehner’s picture

We do not longer use the subquery, as we removed support for the depth again.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community

yes, there is no longer a subquery. just manually tested it to make sure:

SELECT node_field_data.sticky AS node_field_data_sticky, node_field_data.created AS node_field_data_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
INNER JOIN {taxonomy_index} taxonomy_index ON node.nid = taxonomy_index.nid
WHERE (( (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (taxonomy_index.tid = '1') ))
ORDER BY node_field_data_sticky DESC, node_field_data_created DESC
LIMIT 11 OFFSET 0

Now that the depth was removed, the subquery is no longer there.
back to catch

catch’s picture

Status: Reviewed & tested by the community » Needs work

Sorry I missed this first time 'round but it also concerns me.

WHERE (( (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (taxonomy_index.tid = '1') ))

Is this the 'published or admin' filter or similar? Whichever it is it means the caching granularity for the view (as well as the MySQL query cache) becomes per-user.

If we make that just published agreed this is RTBC.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
28.76 KB
905 bytes

Yes, its indeed the "published or admin"
Good point! switched it to just the "Published" filter. here is the query now

SELECT node_field_data.sticky AS node_field_data_sticky, node_field_data.created AS node_field_data_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
INNER JOIN {taxonomy_index} taxonomy_index ON node.nid = taxonomy_index.nid
WHERE (( (node_field_data.status = '1') AND (taxonomy_index.tid = '1') ))
ORDER BY node_field_data_sticky DESC, node_field_data_created DESC
LIMIT 11 OFFSET 0
catch’s picture

Status: Needs review » Needs work

Sorry each time the query gets better, I spot something else...

{taxonomy_index} only includes published nodes - the whole point of that table is to avoid the conditions and/or sorts across two tables. For the same reason it has sticky and created columns denormalized from node. So we ought to be able to 1. remove the status filter 2. have the ORDER BY on that table.

That would get us back more or less to this:

SELECT t.nid AS nid, t.tid AS tid, t.sticky AS sticky, t.created AS created FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0) ORDER BY t.sticky DESC, t.created DESC LIMIT 10 OFFSET 0

.

ParisLiakos’s picture

FileSize
30.01 KB
2.54 KB

tbh i never noticed that taxonomy_index table had created and sticky fields:)
new query:

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 {taxonomy_index} taxonomy_index ON node.nid = taxonomy_index.nid
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
WHERE (( (taxonomy_index.tid = '1') ))
ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC
LIMIT 11 OFFSET 0

If the langcode is removed then there is one less join the one to node_field_data but i am not sure if its a good idea

ParisLiakos’s picture

Status: Needs work » Needs review
catch’s picture

What's adding the langcode to the query?

ParisLiakos’s picture

i am not sure, but it has something to do with the display plugin, since the feed's query doesnt include a langcode and looks like this

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 15 OFFSET 0
Berdir’s picture

Yeah, that's the display with the setting that allows you to control how to deal with translations, I think that is the default setting that displays the node in every translation. I *think* that switching to displaying it in the current UI language would get rid of that and it would make more sense. But that is the same behavior as the frontpage has now (which has an issue to change it).

dawehner’s picture

I guess berdir is talking about #2161845: Node views (front page, admin) do not use the proper language filter

Given that the expected query in #1857256-142: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views can't really be achieved I wonder what we can do here or just skip the conversion and live with it.

ParisLiakos’s picture

FileSize
29.49 KB

hopefully this moves on.
here is a reroll for now

dawehner’s picture

We also have to take into account that currently HEAD does not use an entity query, so it is not really a fair comparison

jibran queued 149: vdc-1857256-149.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 149: vdc-1857256-149.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
28.76 KB

Just helping with a reroll. I think I am out of touch on this issue. I will try and look once it goes through testing.

Status: Needs review » Needs work

The last submitted patch, 153: convert_the_taxonomy-views-1857256-153.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 153: convert_the_taxonomy-views-1857256-153.patch, failed testing.

jhodgdon’s picture

I have not read the 156 comments here to see if this has already been addressed... But please see #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors -- make sure to test this with the Language module enabled, because the current taxonomy view doesn't work with Language enabled.

dawehner’s picture

Status: Needs work » Needs review
FileSize
28.68 KB
1.12 KB

Reroll for now.

Status: Needs review » Needs work

The last submitted patch, 158: 1857256-views-158.patch, failed testing.

jhodgdon’s picture

Note: the error on the other issue is with Content Translation enabled, not just Language.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.21 KB
754 bytes

Let's see how much fails are fixed by that.

jibran’s picture

I think we should add the tests added in #2324809-10: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors to this patch because there is no tests for the taxonomy term view page and its translation.

Status: Needs review » Needs work

The last submitted patch, 161: taxonomy_term-1857256-161.patch, failed testing.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
32.45 KB
5.17 KB

Let's see whether this fixes is already.

@jibran
Do you want to merge the tests?

jibran’s picture

Category: Task » Bug report
Priority: Major » Critical
Issue summary: View changes
Issue tags: +D8MI, +language-content
Related issues: +#2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors
FileSize
33.15 KB
3.94 KB

I am bumping this critical and bug according to #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors because the test fails in that issue are fixed by the changes made in this patch and closing that as a duplicate of this. 2324809-test-only.patch should be green here.

jibran’s picture

The last submitted patch, 165: taxonomy_term-1857256-164.patch, failed testing.

The last submitted patch, 165: 2324809-test-only.patch, failed testing.

dawehner’s picture

+++ b/core/modules/taxonomy/src/Tests/TaxonomyTermViewTest.php
@@ -0,0 +1,117 @@
+  /**
+   * Tests that the taxonomy term view is working properly.
+   */
+  function testTaxonomyTermView() {

The testname and the documentation should explain that this ensures that the view is working fine together with translations.

larowlan’s picture

Quick review, more later

  1. +++ b/core/modules/taxonomy/src/Tests/TaxonomyTermViewTest.php
    @@ -0,0 +1,117 @@
    +  function testTaxonomyTermView() {
    

    needs scope modifier public

  2. +++ b/core/modules/taxonomy/src/Tests/TermIndexTest.php
    @@ -202,6 +202,7 @@ function testTaxonomyIndex() {
    +    \Drupal::moduleHandler()->install(array('views'));
    
    +++ b/core/modules/taxonomy/src/Tests/TermTest.php
    @@ -347,6 +347,7 @@ function testTermAutocompletion() {
    +    \Drupal::moduleHandler()->install(array('views'));
    

    Why not add these to protected static $modules?

Gábor Hojtsy’s picture

I see this resolves the fatal from #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors by removing the depth argument currently in the view (but that is not THE reason to remove the depth argument, so yay for yet another good side effect), but #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors is not resolved for the case when someone does want to do that or override other entity pages with views that may have more dynamic arguments. So I think #2324809: If entity canonical path is overridden with a view with optional arguments, content translation's routes will result in fatal errors is still worth reopening on a lower priority level.

dawehner’s picture

Why not add these to protected static $modules?

Well, I just don't want to enable views for all the tests.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks good except #170/1 and the following:

  1. +++ b/core/modules/taxonomy/src/Tests/TaxonomyTermViewTest.php
    @@ -0,0 +1,117 @@
    +    $this->drupalPostForm('node/' . $node->id() . '/translations/add/en/ur', $edit, t('Save (this translation)'));
    +
    +    /** @var \Drupal\views\ViewStorageInterface $view */
    +    $view = View::load('taxonomy_term');
    +    $view->enable()->save();
    +
    +    $this->drupalGet('taxonomy/term/' . $term->id());
    +    $this->assertText($term->label());
    +    $this->assertText($node->label());
    

    So this does not actually test the translated node shows up on a translated page because a translated page is never requested. Also the translation has no difference from the base content ($edit is the same). Is this just to ensure that it will not blow up? It looks to me like it would be very simple to make the translation a bit different in $edit and test that requesting the translated page shows the right translation of the node.

  2. +++ b/core/modules/node/node.module
    @@ -913,100 +913,6 @@ function node_get_recent($number = 10) {
    - * Page callback: Generates and prints an RSS feed.
    ...
    -function node_feed($nids = FALSE, $channel = array()) {
    

    Is this not used anymore anywhere? Was this not used for the front page / main RSS feed? Is there a replacement for that?

  3. +++ b/core/modules/taxonomy/taxonomy.module
    @@ -126,6 +127,36 @@ function taxonomy_term_uri($term) {
    +    foreach ($term->uriRelationships() as $rel) {
    +      // Set the term path as the canonical URL to prevent duplicate content.
    +      $page['#attached']['drupal_add_html_head_link'][] = array(
    +        array(
    +          'rel' => $rel,
    +          'href' => $term->url($rel),
    +        ),
    +        TRUE,
    +      );
    

    The comment does not seem to match the code. Also it looks odd this needs to be done? Why?

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.35 KB
1.97 KB

So this does not actually test the translated node shows up on a translated page because a translated page is never requested. Also the translation has no difference from the base content ($edit is the same). Is this just to ensure that it will not blow up? It looks to me like it would be very simple to make the translation a bit different in $edit and test that requesting the translated page shows the right translation of the node.

Good catch, let's translate the value, but also ensure that we use random string rather than a random machine name.
Now this though fails, because we don't have the language filter enabled by default, so we are blocked here on the views data issue as well.

Is this not used anymore anywhere? Was this not used for the front page / main RSS feed? Is there a replacement for that?

node_feed() used to be used for /rss.xml but as that is a view, it was just "abused" for taxonomy/term/feed
I don't think we need to provide a similar API as node_feed, especially because zend feed component is actually way more superior already.

Is the new documentation better?

Gábor Hojtsy’s picture

@dawehner: Changes look great. For taxonomy language filtering, my understanding from #2320743: Taxonomy views needs filter/field on original language is that taxonomy does have a translation language filter, which can be used. It only lacks an original language filter, which we don't want to use here, right?

dawehner’s picture

@dawehner: Changes look great. For taxonomy language filtering, my understanding from #2320743: Taxonomy views needs filter/field on original language is that taxonomy does have a translation language filter, which can be used. It only lacks an original language filter, which we don't want to use here, right?

Please take in mind that you actually still looking at nodes on taxonomy/{taxonomy_term} and only nodes (unrelated follow up issue: #2334011: Document that taxonomy/term is just aware of nodes) so the needed language filter here is indeed the one for nodes, which itself is postponed, on the views data issue.

So well indeed we don't want to use the language filter for taxonomy here.

Gábor Hojtsy’s picture

Right, its for nodes. The front page and the node admin views already have the language filter we would use here then. Also node provided views blocks almost have them (#2323899: Provided default Node views need language filtering). So I don't believe we need to postpone this at all?

dawehner’s picture

Right, its for nodes. The front page and the node admin views already have the language filter we would use here then. Also node provided views blocks almost have them (#2323899: Provided default Node views need language filtering). So I don't believe we need to postpone this at all?

Okay let's do it, this issue is critical enough already.

Gábor Hojtsy’s picture

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

Yay, looks great! Thanks so much for working this out :)

dawehner’s picture

Here is what you could have reviewed.

  1. +++ b/core/modules/taxonomy/taxonomy.module
    @@ -333,8 +365,8 @@ function template_preprocess_taxonomy_term(&$variables) {
    +  if (in_array(\Drupal::routeMatch()->getRouteName(), ['view.taxonomy_term.page_1', 'entity.taxonomy_term.canonical']) && $page_term_id = \Drupal::routeMatch()->getRawParameter('taxonomy_term')) {
    +    return $page_term_id == $term->id();
    

    Are we sure that this is right? I don't think so. The ID of the view should be entity.taxonomy_term.canonical

  2. +++ b/core/modules/views/src/EventSubscriber/RouteSubscriber.php
    @@ -87,6 +87,8 @@ public static function getSubscribedEvents() {
         $events[RoutingEvents::FINISHED] = array('routeRebuildFinished');
    +    $events[RoutingEvents::ALTER] = ['onAlterRoutes', -175];
    

    This should be documented why we need to come such late.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
catch’s picture

Also could someone post before/after with the query again, ideally with an EXPLAIN? Doesn't look like that happened since #128.

xjm’s picture

Assigned: Unassigned » xjm

Not sure about #180 but I'll get the EXPLAINs.

xjm’s picture

Status: Needs work » Needs review
FileSize
32.37 KB

Patch needed a reroll; didn't incorporate #180.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs change record updates
FileSize
17.32 KB
15.45 KB

Queries before conversion

SELECT COUNT(t.nid) AS expression FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0)
SELECT t.nid AS nid, t.tid AS tid, t.sticky AS sticky, t.created AS created FROM taxonomy_index t WHERE (tid = :db_condition_placeholder_0) ORDER BY t.sticky DESC, t.created DESC LIMIT 10 OFFSET 0

Query after conversion

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 taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid WHERE (( (taxonomy_index.tid = :taxonomy_index_tid) )) ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC LIMIT 11 OFFSET 0

Edit: Pasted wrong query :)

xjm’s picture

Updated https://www.drupal.org/node/2084727 to remove the reference to the depth argument (which is not a part of D7 core).

xjm’s picture

Aside, taxonomy_term_load_parents(), which is called both in HEAD and with the patch before the view is built is doing a filesort. But the patch doesn't touch that part of the page build.

xjm’s picture

Assigned: xjm » Unassigned
dawehner’s picture

Status: Needs work » Needs review
FileSize
36.13 KB
5.18 KB

Rule 1: don't trust yourself

  • If you look at the router after you have reinstalled drupal with this patch you will realize that there will be two routes with the same path but same configuration otherwise, which means that Drupal might decide to pick of views.taxonomy_term.page_1 (see review #180)
  • Ensure to drop the original route with the name provided by views
  • Expand the test coverage for that.
  • @catch You might could complain about the join on the node table, which is kind of not needed, you are absolute right here. After #1740492: Implement a default entity views data handler we plan to actually never use the normal base table but instead rely on {}_field_data if available.
tim.plunkett’s picture

+++ b/core/modules/views/src/EventSubscriber/RouteSubscriber.php
@@ -170,7 +172,9 @@ protected function alterRoutes(RouteCollection $collection) {
+              $collection->remove("views.$view_route_name");

Wowww. @dawehner++

jibran’s picture

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -0,0 +1,126 @@
+ * Contains \Drupal\taxonomy\Tests\Views\TaxonomyTermViewTest.

This test is missing from current patch.

jibran’s picture

Re-add the test.

xjm’s picture

The query after #1740492: Implement a default entity views data handler looks the same:

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 taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid INNER JOIN node_field_data node_field_data ON node.nid = node_field_data.nid WHERE (( (taxonomy_index.tid = :taxonomy_index_tid) )) ORDER BY taxonomy_index_sticky DESC, taxonomy_index_created DESC LIMIT 11 OFFSET 0

@dawehner, is there a separate Views followup to rely only on the field data table?

xjm’s picture

That's my fault; I rerolled #174 instead of #178 I think. Nice catch @jibran!

Attached reincorporates all of #178. Interdiff is against @dawehner's patch from #194 (so includes @jibran's partial fix).

xjm’s picture

Crossposting with myself... good times.

The last submitted patch, 197: taxonomy_term-1857256-197.patch, failed testing.

jibran’s picture

+++ b/core/modules/taxonomy/config/install/views.view.taxonomy_term.yml
@@ -1,8 +1,16 @@
+status: true

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -0,0 +1,126 @@
+    /** @var \Drupal\views\ViewStorageInterface $view */
+    $view = View::load('taxonomy_term');
+    $view->enable()->save();

If status is true then I think we can remove these view enable lines.

dawehner’s picture

FileSize
759 bytes
40.72 KB

jibran++++++ seriously good catch!

@dawehner, is there a separate Views followup to rely only on the field data table?

Nope, not yet.

jibran’s picture

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -0,0 +1,122 @@
+use Drupal\views\Entity\View;

We can remove this as well ;)

Status: Needs review » Needs work

The last submitted patch, 203: vdc-1857256-203.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
40.17 KB
1.58 KB

Fixed the merge conflict.

xjm’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Nice, this is looking good. Re-TBC I think? @dawehner addressed all @dawehner's feedback ;) and answers re: queries for @catch's review are in the summary.

catch’s picture

With the follow-up to clean up that query it's OK, indexes are fine at least. Thanks for posting the explain and linking to the follow-ups.

Couple of things though:

bar when viewing the listing page).") . '</dd>';
+      $output .= '<dt>' . t('Viewing listings') . '</dt>';
+      $output .= '<dd>' . t("Each taxonomy term automatically provides a page listing content that has its classificatio. For example, if the taxonomy term <em>country rock</em> has the ID 123 (you can see this by looking at the URL when hovering on the linked term, which you can click to navigate to the listing page), then you will find this list at the path <em>taxonomy/term/123</em>.") . '</dd>';

'classificatio'

In general the help text is quite clunky here but that seems inherited from the existing text.

Also I think this needs its own change notice for site builders about the removal of the RSS feed - that's potentially a lot of broken links.

Gábor Hojtsy’s picture

FileSize
40.17 KB

@catch: there is no feed lost here. The patch removes this route:

taxonomy.term_feed:
  path: '/taxonomy/term/{taxonomy_term}/feed'

But the view exposes a feed display at path: taxonomy/term/%/feed (so same path). It does seem to be true that this removed line has no equivalent in the new code:

-  $build['#attached']['drupal_add_feed'][] = array('taxonomy/term/' . $term->id() . '/feed', 'RSS - ' . $term->getName());

HOWEVER #2251121: Support to add a feed icon on pages with parameters was mentioned above as a followup required to do that. Views has the capability to assign the feed to the page display, so it would show the feed, but not if the view has arguments. So it does not seem like a separate change notice is in order at all? Also, there is already https://www.drupal.org/node/2084727 as change notice assigned for site builders, which makes best sense to me.

So looks like this is down to the "classificatio" typo fix. Hand fixed that in this patch.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Yeah the feed itself is still there and now provided by a display of the view; it's just that the icon linking to it is missing. (The icon is a broken link in HEAD.)

The old bit about the changing feed path was reverted when we took out the depth filter.

I removed stale references to the depth filter in the CR and added the bit about the RSS feed icon to the summary; I'd forgotten about that.

xjm’s picture

Issue summary: View changes
webchick’s picture

Assigned: Unassigned » catch

Looks like catch has already been all over this one, so assigning.

jibran’s picture

Issue tags: +Needs reroll
+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTermViewTest.php
@@ -0,0 +1,121 @@
+    $language = new Language(array('id' => 'ur'));

After #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete() this is changed to ConfigurableLanguage::createFromLangcode('ur'); so it just needs a quick reroll.

dawehner’s picture

FileSize
1.01 KB
40.23 KB

Thank you for the pointer!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.0.x

  • catch committed 983cdbf on 8.0.x
    Issue #1857256 by dawehner, xjm, tim.plunkett, jibran, ParisLiakos,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Amazing, thanks a lot all!

dawehner’s picture

Cool, thank you!

ParisLiakos’s picture

Yay! thanks everyone for finishing this one!

jibran’s picture

Issue tags: -Needs reroll

Finally. We have to update some change notices.

Thank you for committing this almost everyone worked on this issue :)

Status: Fixed » Closed (fixed)

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

larowlan’s picture

Related follow-up

#2888756: Crawl errors in forum page because of feed url

Forum was unconditionally adding a link to the RSS feed to the page HEAD (as well as doing it wrong, but that's another story).

But the feed is only available if views is enabled (the route was removed here).

Reviews welcome.