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.
We do not want to have hardcoded breadcrumb, since any other module would be able to override it.
Comment | File | Size | Author |
---|---|---|---|
#13 | 1677694-13.remove_custom_breadcrumbs.patch | 16.69 KB | rszrama |
#12 | 1677694-remove-hardcoded-breadcrumbs.patch | 6.94 KB | bojanz |
#1 | 1677694-commerce-remove_breadcrumb.patch | 3.9 KB | theo_ |
Comments
Comment #1
theo_ CreditAttribution: theo_ commentedHere is the patch
Comment #2
theo_ CreditAttribution: theo_ commentedComment #4
rszrama CreditAttribution: rszrama commentedCan another module not simply set the breadcrumb after these modules?
Comment #5
theo_ CreditAttribution: theo_ commentedYes, sure. Since we removed hardcoded breadcrumb it became easy to set a breadcrumb anywhere.
Comment #6
rszrama CreditAttribution: rszrama commentedWell, I mean, even if we don't apply this patch, is there a problem with overriding the breadcrumb settings from the UI modules? The problem is that without these breadcrumb settings in our modules, the breadcrumb for our UI pages was always wrong. Maybe that's changed in Drupal core since we first implemented it. : ?
Comment #7
bojanz CreditAttribution: bojanz commented#1: 1677694-commerce-remove_breadcrumb.patch queued for re-testing.
Comment #9
bojanz CreditAttribution: bojanz commentedTagging.
Will update the issue summary and try to get the tests to pass soon.
Comment #10
rszrama CreditAttribution: rszrama commentedFor this one, I still don't get why we can't just set a new breadcrumb in a module weighted after our UI modules. Additionally, if we're going to deprecate something, we'd have to mark it as deprecated and then remove it two minor versions later. So even if we wanted to deprecate this in 1.4, we wouldn't remove the code until 1.6.
Comment #11
theo_ CreditAttribution: theo_ commentedWe are not able to set a new breadcrumb with a module weighted after our UI modules cause we set breadcrumb directly using drupal_set_breadcrumb() here and there, so it always occur last and there is no way to alter it.
So we should move our breadcrumb alteration into a hook_menu_breadcrumb_alter() then everyone will be happy :)
Comment #12
bojanz CreditAttribution: bojanz commentedReuploading patch with the offending test removed. There's no need for it anymore.
Kickstart doesn't take breadcrumbs set via drupal_set_breadcrumb() into account at all anymore, so technically this doesn't affect us anymore.
Still, doing this is the right thing.
These functions were written at a time when core breadcrumbs didn't work.
Now they work, and they work better than the Commerce functions, which often set the wrong breadcrumb.
That's one problem.
The other problem is that we are promoting a false best practice. Many contrib modules have copied Commerce and are setting their breadcrumbs manually for no good reason, leading to more code with more bugs (since doing breadcrumbs properly isn't really easy, and most developers don't take the time to understand all the finer points.).
So, here are Commerce breadcrumbs before and after the patch:
As you can see, main pages (and default local tasks) are unaffected. The breadcrumbs stay the same.
Other local tasks (such as the delete pages) get the correct breadcrumb now, matching the rest of core.
I think this still fits the definition. We are leaving the functions themselves so that people's code doesn't blow up, while changing what's inside in order to fix bugs (illustrated in the table above).
Comment #13
rszrama CreditAttribution: rszrama commentedIn light of such overwhelming evidence, I am forced to concede the point. ; )
Really, thanks for digging through this and providing the comparison chart. I figure if we're going to deprecate these functions, we should also update our code to remove any calls to them. This also involved changing a couple function signatures to remove their terminal arguments that dealt with toggling breadcrumbs, a forward compatible change.
TestBot, Hooo!
Comment #14
rszrama CreditAttribution: rszrama commentedCommitted!
Comment #16
pirex360 CreditAttribution: pirex360 commentedSorry for reopening...
But i cant apply the patch in the new kickstart version....
Comment #17
bojanz CreditAttribution: bojanz commentedThe change was committed, made it into Commerce 1.4, which is included in Kickstart.