Files: 
CommentFileSizeAuthor
#32 drupal8.taxonomy-module.2061911-32.patch2.91 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,822 pass(es).
[ View ]
#32 interdiff.txt822 bytesjibran
#30 drupal8.taxonomy-module.2061911-30.patch5.66 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,788 pass(es).
[ View ]
#30 interdiff.txt508 bytesjibran
#26 drupal8.taxonomy-module.2061911-26.patch5.65 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,629 pass(es).
[ View ]
#26 interdiff.txt1.17 KBjibran
#24 drupal8.taxonomy-module.2061911-24.patch5.48 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]
#24 interdiff.txt577 bytesjibran
#22 drupal8.taxonomy-module.2061911-22.patch20.48 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,731 pass(es).
[ View ]
#22 interdiff.txt15.56 KBjibran
#20 drupal8.taxonomy-module.2061911-20.patch5.52 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es).
[ View ]
#20 interdiff.txt1.75 KBjibran
#18 taxonomy-2061911-18.patch6.28 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,367 pass(es), 483 fail(s), and 284 exception(s).
[ View ]
#17 taxonomy-2061911-17.patch3.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#12 convert_breadcrumb-taxonomy-2061911-6.patch2.7 KBalextdk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_breadcrumb-taxonomy-2061911-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 convert_breadcrumb-taxonomy-2061911-5.patch2.68 KBalextdk
FAILED: [[SimpleTest]]: [MySQL] 58,201 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 convert_breadcrumb-taxonomy-2061911-4.patch2.69 KBalextdk
FAILED: [[SimpleTest]]: [MySQL] 58,174 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#5 convert_breadcrumb-taxonomy-2061911-3.patch2.69 KBalextdk
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 convert_breadcrumb-taxonomy-2061911-2.patch2.7 KBalextdk
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 convert_breadcrumb-taxonomy-2061911-1.patch3.13 KBalextdk
PASSED: [[SimpleTest]]: [MySQL] 57,874 pass(es).
[ View ]

Comments

Assigned:Unassigned» alextdk
Status:Active» Needs review
StatusFileSize
new3.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,874 pass(es).
[ View ]

@@ -0,0 +1,45 @@
+  ¶
...
+        ¶
...
+        //   ¶
+        // this code and todo comment copy from taxonomy.pages.inc, dont know ¶
...
+        ¶
...
+        $current = $item['map'][2];        ¶
...
+        ¶
@@ -4,3 +4,8 @@ services:
+      ¶
\ No newline at end of file

trailing white spaces
and add new line in the end

StatusFileSize
new2.7 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

@@ -0,0 +1,38 @@
+    if (isset($attributes['_drupal_menu_item']) && ¶
+        $attributes['_drupal_menu_item']['path'] == 'taxonomy/term/%') {

please remove trailing whitespace and make the condition one-line

@@ -0,0 +1,38 @@
+        $breadcrumb[] = l($current->label(), 'taxonomy/term/' . $current->id());
...
+      $breadcrumb[] = l(t('Home'), NULL);

Probably it needs url.generator service injection, but not sure

StatusFileSize
new2.69 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

clear whitespaces

StatusFileSize
new2.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,174 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

The last submitted patch, convert_breadcrumb-taxonomy-2061911-4.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, convert_breadcrumb-taxonomy-2061911-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,201 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, convert_breadcrumb-taxonomy-2061911-5.patch, failed testing.

StatusFileSize
new2.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_breadcrumb-taxonomy-2061911-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs work» Needs review

Status:Needs review» Needs work

Pretty much RTBC. Minor issues.

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermBreadcrumbBuilder.php
    @@ -0,0 +1,38 @@
    +    if (isset($attributes['_drupal_menu_item']) && $attributes['_drupal_menu_item']['path'] == 'taxonomy/term/%') {
    ...
    +      $current = $attributes['_drupal_menu_item']['map'][2];

    We should add todo related to #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermBreadcrumbBuilder.php
    @@ -0,0 +1,38 @@
    +      while ($parents = taxonomy_term_load_parents($current->id())) {

    Can we add todo here and create follow up issue to convert taxonomy_term_load_parents to TaxonomyManger method if there not already an existing issue?

Related issues #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views and #2061913: Change notice: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in Views module

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

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

The last submitted patch, convert_breadcrumb-taxonomy-2061911-6.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience)
StatusFileSize
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Let's introduce a new base class to have t() and l() available

StatusFileSize
new6.28 KB
FAILED: [[SimpleTest]]: [MySQL] 57,367 pass(es), 483 fail(s), and 284 exception(s).
[ View ]

Forgot a new file.

Status:Needs review» Needs work

The last submitted patch, taxonomy-2061911-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
new5.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es).
[ View ]

Reverted ControllerBase changes.

Ehem, ups!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermBreadcrumbBuilder.php
@@ -7,6 +7,7 @@
+use Drupal\taxonomy\TermInterface;

This is not needed as it is part of the same namespace.

Title:Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in taxonomy moduleRemove drupal_set_breadcrumb from taxonomy module and add BreadcrumbBuilderBase
Assigned:alextdk» Unassigned
StatusFileSize
new15.56 KB
new20.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,731 pass(es).
[ View ]

@dawehner you want to go that road (1 line reviews :P). How is this for the review? :D

rtbc +1, seems better to file separate issue for BreadcrumbBuilderBase

Title:Remove drupal_set_breadcrumb from taxonomy module and add BreadcrumbBuilderBaseRemove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in taxonomy module
StatusFileSize
new577 bytes
new5.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]

Here is the patch without all crazy changes. Created #2099095: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders for crazy changes.

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermBreadcrumbBuilder.php
    @@ -0,0 +1,37 @@
    + * Definition of Drupal\taxonomy\TermBreadcrumbBuilder.

    Contains ...

  2. +++ b/core/modules/taxonomy/taxonomy.pages.inc
    @@ -19,17 +19,6 @@ function taxonomy_term_page(Term $term) {
    -  // @todo This overrides any other possible breadcrumb and is a pure hard-coded
    -  //   presumption. Make this behavior configurable per vocabulary or term.
    -  $breadcrumb = array();

    I am wondering whether we want to drop that comment.

StatusFileSize
new1.17 KB
new5.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,629 pass(es).
[ View ]

Thanks for the review @dawehner :). Fixed #25.

Great this is looking great now but I probably worked too much on this patch to set it to RTBC.

I don't love this pattern, but it seems agreed as of now given that we are staying with 5.3.

Thanks for working on this @jibran. Here is some review comments.

  1. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderBase.php
    @@ -0,0 +1,89 @@
    +    return \Drupal::getContainer();

    Sorry, I may be missing something here, but why aren't we keeping container as property (the same way languageManager() does)?

  2. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderBase.php
    @@ -0,0 +1,89 @@
    +   * Renders a link to a route given a route name and its parameters.

    This comment needs update. Not sure I get it right. But here is what I would try:

    Renders link to given route details.

  3. +++ b/core/lib/Drupal/Core/Breadcrumb/BreadcrumbBuilderBase.php
    @@ -0,0 +1,89 @@
    +   * See the t() documentation for details.

    @see?

  4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermBreadcrumbBuilder.php
    @@ -0,0 +1,39 @@
    + * Contains of Drupal\taxonomy\TermBreadcrumbBuilder.

    just Contains \Drupal

  5. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermBreadcrumbBuilder.php
    @@ -0,0 +1,39 @@
    +  public function build(array $attributes) {
    +    if (!empty($attributes[RouteObjectInterface::ROUTE_NAME]) && $attributes[RouteObjectInterface::ROUTE_NAME] == 'taxonomy.term_page' && ($term = $attributes['taxonomy_term']) && $term instanceof TermInterface) {
    +      // @todo This overrides any other possible breadcrumb and is a pure
    +      //   hard-coded presumption. Make this behavior configurable per
    +      //   vocabulary or term.
    +      $breadcrumb = array();
    +      while ($parents = taxonomy_term_load_parents($term->id())) {
    +        $term = array_shift($parents);
    +        $breadcrumb[] = $this->l($term->label(), 'taxonomy.term_page', array('taxonomy_term' => $term->id()));
    +      }
    +      $breadcrumb[] = $this->l($this->t('Home'), '<front>');
    +      $breadcrumb = array_reverse($breadcrumb);
    +
    +      return $breadcrumb;
    +    }

    Minor suggestions:
    1. Can we define $attributes[RouteObjectInerface::ROUTE_NAME] as a variable and use in IF to get a readable/shorter code?

    2. can we move $breadcrumb initialisation and return out of the if condition, so that we can assure that we always return array from this method (array with elements or array())

  6. +++ b/core/modules/taxonomy/taxonomy.services.yml
    @@ -0,0 +1,5 @@
    +      - { name: breadcrumb_builder, priority: 1002 }

    what is this number 1002 means? can we document/refer somewhere?

StatusFileSize
new508 bytes
new5.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,788 pass(es).
[ View ]

Thank you for the review @vijaycs85.

  1. I don't see any benefit of keeping container as property and currently in HEAD LanguageManager is also not storing container as property.
  2. It is same as ControllerBase::l.
  3. It is same as ControllerBase::t.
  4. Fixed
  5. Well we still have to check empty so I don't think it will help us.
  6. @see BreadcrumbManager::build it has NULL check this is like this to make it backward compatible with drupal_set_breadcrumbs used in LegacyBreadcrumbBuilder::build
  7. It has the priority higher then LegacyBreadcrumbBuilder. You are right it needs doc let's create follow up issue to add doc to all BreadcrumbBuilders

Status:Needs review» Reviewed & tested by the community

2. can we move $breadcrumb initialisation and return out of the if condition, so that we can assure that we always return array from this method (array with elements or array())

Just to clarify that point. If you do return an empty array the breadcrumb manager assumes that you return an empty breadcrumb but NULL is no breadcrumb so another breadcrumb builder can jump in.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new822 bytes
new2.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,822 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Fine even I think that explicit define the interface adds some nice information.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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