Refactor breadcrumb code from #2717537: Breadcrumbs support after solving #2861586: Make it easier to programmatically generate facet links

CommentFileSizeAuthor
#11 2912111-9--refactor-breadcrumb-code-with-new-url-generation.patch3.34 KBStryKaizer
#5 refactor_breadcrumb-2912111-5.patch2.7 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dragos-dumi created an issue. See original summary.

dragos-dumi’s picture

dragos-dumi CreditAttribution: dragos-dumi as a volunteer commented
Parent issue: » #2717537: Breadcrumbs support
Related issues: +#2861586: Make it easier to programmatically generate facet links
dragos-dumi’s picture

dragos-dumi CreditAttribution: dragos-dumi as a volunteer commented
Issue tags: +Vienna2017
dragos-dumi’s picture

dragos-dumi CreditAttribution: dragos-dumi as a volunteer commented
Issue summary: View changes
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Title: Refactor breadcrumb code from #2717537 » Refactor breadcrumb code with new url generation
FileSize
refactor_breadcrumb-2912111-5.patch2.7 KB

In theory this should do it, but it doesn't. Not sure if this is a problem with my refactor or with the actual code we wrote in the getUrl class.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: refactor_breadcrumb-2912111-5.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Priority: Normal » Major

Marking this as major, because it looks like this proves that the url generator class doesn't work as expected.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and at ClicketyClick commented
Status: Needs work » Needs review

Only found this issue after I created a patch, thus no interdiff, sry.

Attached patch will fix breadcrumbs for pretty paths too, as url generation used to be hardcoded to querystring structure.

@borisson_: url processor works as expected, but it generates an URL, not a link, thus we need the displayvalue too, which we still need to retrieve from the result.

StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and at ClicketyClick commented
StryKaizer’s picture

StryKaizer
Primary language Dutch
Location Belgium
CreditAttribution: StryKaizer as a volunteer and at ClicketyClick commented
FileSize
2912111-9--refactor-breadcrumb-code-with-new-url-generation.patch3.34 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This looks very good, thanks!

  • borisson_ committed 478953b on 8.x-1.x authored by StryKaizer 
    Issue #2912111 by borisson_, StryKaizer: Refactor breadcrumb code with...
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Committed this, thanks!

Status: Fixed » Closed (fixed)

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