Remove hardcoded breadcrumbs

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

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

#1: 1677694-commerce-remove_breadcrumb.patch queued for re-testing.

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.

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:

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

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!

Committed!

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.