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. Let's remove the custom code and use the view!

Files: 
CommentFileSizeAuthor
#93 vdc-1857256-93.patch9.86 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,102 pass(es), 19 fail(s), and 1 exception(s).
[ View ]
#88 vdc-1857256-88.patch16.68 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] 63,600 pass(es), 19 fail(s), and 0 exception(s).
[ View ]
#64 vdc-1857256.patch20.21 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,234 pass(es), 101 fail(s), and 33 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new10.82 KB
FAILED: [[SimpleTest]]: [MySQL] 49,275 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.86 KB
new14.47 KB
FAILED: [[SimpleTest]]: [MySQL] 49,323 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new2.53 KB
new13.95 KB
FAILED: [[SimpleTest]]: [MySQL] 49,328 pass(es), 46 fail(s), and 23 exception(s).
[ View ]

You know my friend, learn the power of views.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new18.1 KB
FAILED: [[SimpleTest]]: [MySQL] 49,301 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.05 KB
new17.93 KB
FAILED: [[SimpleTest]]: [MySQL] 49,235 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

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.

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.

Issue summary:View changes

add meta

StatusFileSize
new18.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1857256-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

That's just a rerole.

StatusFileSize
new19.33 KB
FAILED: [[SimpleTest]]: [MySQL] 53,051 pass(es), 36 fail(s), and 10 exception(s).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new19.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1857256-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's use the new area handler now.

StatusFileSize
new41.14 KB
new39.96 KB

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

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

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?

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?

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?

how about using menu_base_path from Term Enity plugin annotation?

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.

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

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.

Title:Convert the taxonomy listing at /taxonomy/term/%term to ViewsConvert 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

Assigned:Unassigned» trevjs

Assigned:trevjs» rdrh555

StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] 57,562 pass(es), 11 fail(s), and 1 exception(s).
[ View ]

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.

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)

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.

Status:Needs work» Needs review
StatusFileSize
new2.2 KB
new2.93 KB
FAILED: [[SimpleTest]]: [MySQL] 57,533 pass(es), 1 fail(s), and 580 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new520 bytes
new3.17 KB
FAILED: [[SimpleTest]]: [MySQL] 57,647 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Missed one change.

Status:Needs review» Needs work

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

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

StatusFileSize
new20.11 KB
FAILED: [[SimpleTest]]: [MySQL] 58,471 pass(es), 37 fail(s), and 10 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new26.84 KB
FAILED: [[SimpleTest]]: [MySQL] 57,897 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

+++ 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?! :(

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.

Assigned:rdrh555» Unassigned

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

Issue summary:View changes

Correct meta issue.

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

Status:Needs work» Needs review
StatusFileSize
new26.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,555 pass(es), 153 fail(s), and 21 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new8.6 KB
new30.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,934 pass(es), 63 fail(s), and 13 exception(s).
[ View ]

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.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new20.83 KB
FAILED: [[SimpleTest]]: [MySQL] 59,186 pass(es), 38 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new20.21 KB
FAILED: [[SimpleTest]]: [MySQL] 59,234 pass(es), 101 fail(s), and 33 exception(s).
[ View ]
new11.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.

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

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

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.

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

Tagging for reroll.

Assigned:Unassigned» rhabbachi
Issue tags:+TUNIS_2014_JANUARY

Status:Needs work» Needs review
StatusFileSize
new36.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-1857256-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

StatusFileSize
new41.65 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new32.49 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

StatusFileSize
new20.15 KB
FAILED: [[SimpleTest]]: [MySQL] 61,893 pass(es), 827 fail(s), and 44 exception(s).
[ View ]

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.

Status:Needs work» Needs review

+++ 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: Change record: EntityInterface::uri() should use route name and not path, but only halfway.

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.

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

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: Change record: 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.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new16.68 KB
FAILED: [[SimpleTest]]: [MySQL] 63,600 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new9.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,102 pass(es), 19 fail(s), and 1 exception(s).
[ View ]

Let's see how much is broken.

Status:Needs review» Needs work

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

Issue tags:+beta target