Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #2026075: [meta] Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder
All places in taxonomy should be fixed
Comment | File | Size | Author |
---|---|---|---|
#32 | drupal8.taxonomy-module.2061911-32.patch | 2.91 KB | jibran |
#32 | interdiff.txt | 822 bytes | jibran |
#30 | drupal8.taxonomy-module.2061911-30.patch | 5.66 KB | jibran |
#30 | interdiff.txt | 508 bytes | jibran |
#26 | drupal8.taxonomy-module.2061911-26.patch | 5.65 KB | jibran |
Comments
Comment #1
alextdk CreditAttribution: alextdk commentedComment #2
andyposttrailing white spaces
and add new line in the end
Comment #3
alextdk CreditAttribution: alextdk commentedComment #4
andypostplease remove trailing whitespace and make the condition one-line
Probably it needs url.generator service injection, but not sure
Comment #5
alextdk CreditAttribution: alextdk commentedclear whitespaces
Comment #6
alextdk CreditAttribution: alextdk commentedComment #8
AndreyMaximov CreditAttribution: AndreyMaximov commented#6: convert_breadcrumb-taxonomy-2061911-4.patch queued for re-testing.
Comment #10
alextdk CreditAttribution: alextdk commentedComment #12
alextdk CreditAttribution: alextdk commentedComment #13
alextdk CreditAttribution: alextdk commentedComment #14
jibranPretty much RTBC. Minor issues.
We should add todo related to #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views
Can we add todo here and create follow up issue to convert
taxonomy_term_load_parents
toTaxonomyManger
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
Comment #15
jibran#12: convert_breadcrumb-taxonomy-2061911-6.patch queued for re-testing.
Comment #17
dawehnerLet's introduce a new base class to have t() and l() available
Comment #18
dawehnerForgot a new file.
Comment #20
jibranReverted ControllerBase changes.
Comment #21
dawehnerEhem, ups!
This is not needed as it is part of the same namespace.
Comment #22
jibran@dawehner you want to go that road (1 line reviews :P). How is this for the review? :D
Comment #23
andypostrtbc +1, seems better to file separate issue for
BreadcrumbBuilderBase
Comment #24
jibranHere is the patch without all crazy changes. Created #2099095: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders for crazy changes.
Comment #25
dawehnerContains ...
I am wondering whether we want to drop that comment.
Comment #26
jibranThanks for the review @dawehner :). Fixed #25.
Comment #27
dawehnerGreat this is looking great now but I probably worked too much on this patch to set it to RTBC.
Comment #28
pwolanin CreditAttribution: pwolanin commentedI don't love this pattern, but it seems agreed as of now given that we are staying with 5.3.
Comment #29
vijaycs85Thanks for working on this @jibran. Here is some review comments.
Sorry, I may be missing something here, but why aren't we keeping container as property (the same way languageManager() does)?
This comment needs update. Not sure I get it right. But here is what I would try:
Renders link to given route details.
@see?
just Contains \Drupal
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())
what is this number 1002 means? can we document/refer somewhere?
Comment #30
jibranThank you for the review @vijaycs85.
LanguageManager
is also not storing container as property.LegacyBreadcrumbBuilder
. You are right it needs doc let's create follow up issue to add doc to allBreadcrumbBuilders
Comment #31
dawehnerJust 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.
Comment #32
jibranReroll after #2099095: Create BreadcrumbBuilderBase and remove the boilerplate code from BreadcrumbBuilders.
Comment #33
dawehnerFine even I think that explicit define the interface adds some nice information.
Comment #34
catchCommitted/pushed to 8.x, thanks!