We do not want to have hardcoded breadcrumb, since any other module would be able to override it.

Files: 
CommentFileSizeAuthor
#13 1677694-13.remove_custom_breadcrumbs.patch16.69 KBrszrama
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es).
[ View ]
#12 1677694-remove-hardcoded-breadcrumbs.patch6.94 KBbojanz
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es).
[ View ]
#1 1677694-commerce-remove_breadcrumb.patch3.9 KBtheo_
FAILED: [[SimpleTest]]: [MySQL] 3,581 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Comments

StatusFileSize
new3.9 KB
FAILED: [[SimpleTest]]: [MySQL] 3,581 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Here is the patch

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, 1677694-commerce-remove_breadcrumb.patch, failed testing.

Can another module not simply set the breadcrumb after these modules?

Yes, sure. Since we removed hardcoded breadcrumb it became easy to set a breadcrumb anywhere.

Well, 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. : ?

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1677694-commerce-remove_breadcrumb.patch, failed testing.

Issue tags:+kickstart blocker

Tagging.

Will update the issue summary and try to get the tests to pass soon.

For 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.

We 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 :)

Title:Remove hardcoded breadcrumbRemove hardcoded breadcrumbs
Assigned:theo_» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.94 KB
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es).
[ View ]

Reuploading 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:

URL Breadcrumb before Breadcrumb after
admin/commerce/products/1/edit Home > Administration > Store > Products Home > Administration > Store > Products
admin/commerce/products/1/delete Home > Administration > Store > Products Home > Administration > Store > Products > Product: PROD-01
admin/commerce/products/types/product/edit Home > Administration > Store > Products > Product Types Home > Administration > Store > Products > Product Types
admin/commerce/products/types/product/delete Home > Administration > Store > Products > Product Types Home > Administration > Store > Products > Product Types > Product
admin/commerce/orders/2 Home > Administration > Store > Orders Home > Administration > Store > Orders
admin/commerce/orders/2/edit (and delete) Home > Administration > Store > Orders Home > Administration > Store > Orders > Order 2
admin/commerce/orders/2/payment/1/view Home > Administration > Store > Orders > Order 2 > Payment Home > Administration > Store > Orders > Order 2 > Payment
admin/commerce/orders/2/payment/1/delete Home > Administration > Store > Orders > Order 2 > Payment Home > Administration > Store > Orders > Order 2 > Payment > Transaction 1
admin/commerce/customer-profiles/1/edit Home > Administration > Store > Customer profiles Home > Administration > Store > Customer profiles
admin/commerce/customer-profiles/1/delete Home > Administration > Store > Customer profiles > Profile Types (?!) Home > Administration > Store > Customer profiles > Customer profile 1

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.

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.

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).

Component:Other» Developer experience
Issue tags:-kickstart blocker
StatusFileSize
new16.69 KB
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es).
[ View ]

In 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!

Status:Needs review» Fixed

Committed!

Status:Fixed» Closed (fixed)

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

Sorry for reopening...

But i cant apply the patch in the new kickstart version....

The change was committed, made it into Commerce 1.4, which is included in Kickstart.