Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alextdk’s picture

Assigned: Unassigned » alextdk
Status: Active » Needs review
FileSize
3.13 KB
andypost’s picture

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

alextdk’s picture

andypost’s picture

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

alextdk’s picture

clear whitespaces

alextdk’s picture

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

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

AndreyMaximov’s picture

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.

alextdk’s picture

Status: Needs work » Needs review
FileSize
2.68 KB

Status: Needs review » Needs work

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

alextdk’s picture

alextdk’s picture

Status: Needs work » Needs review
jibran’s picture

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: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in Views module

jibran’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)
FileSize
3.63 KB

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

dawehner’s picture

FileSize
6.28 KB

Forgot a new file.

Status: Needs review » Needs work

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

jibran’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
5.52 KB

Reverted ControllerBase changes.

dawehner’s picture

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.

jibran’s picture

Title: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in taxonomy module » Remove drupal_set_breadcrumb from taxonomy module and add BreadcrumbBuilderBase
Assigned: alextdk » Unassigned
FileSize
15.56 KB
20.48 KB

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

andypost’s picture

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

jibran’s picture

Title: Remove drupal_set_breadcrumb from taxonomy module and add BreadcrumbBuilderBase » Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in taxonomy module
FileSize
577 bytes
5.48 KB

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

dawehner’s picture

  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.

jibran’s picture

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

dawehner’s picture

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

pwolanin’s picture

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

vijaycs85’s picture

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?

jibran’s picture

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
dawehner’s picture

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.

jibran’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
822 bytes
2.91 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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