Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in Views module

@AndreyMaximov, how to test? The patch applies cleanly and I would like to review and push this through. I have built several test views, where are the breadcrumbs?

Been watching that thread, trying to understand it. Maybe work with dawehner before coming back to this thread.

Been watching that thread, trying to understand it. Maybe work with dawehner before coming back to this thread.

Tagging. #2026073: Convert drupal_set_breadcrumb/ViewExecutable::getBreadcrumb() to breadcrumb builder service in ViewExecutable is marked as duplicate.

A different possibility would be to remove the breadcrumb support out of views and create a special one for taxonomy.
On an architecture point of view this totally makes sense, as the breadcrumb system is then separate from views.

Adding API Change tag do get feedback from one maintainer.

something like #2061911: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in taxonomy module?

Also related #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views

Tagging as part of #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff.

@xjm
Did you approved the issue or my idea in #2061913-11: Remove drupal_set_breadcrumb and LegacyBreadcrumbBuilder in Views module ?

@dawehner I think \Drupal\views\Plugin\views\display\Page::execute() just needs a bit refactoring,
there's no other usage of getBreadcrumb():

$ git grep getBreadcrumb core/modules/views
core/modules/views/lib/Drupal/views/Plugin/views/display/Page.php:87:    $this->view->getBreadcrumb(TRUE);
core/modules/views/lib/Drupal/views/ViewExecutable.php:1705:  public function getBreadcrumb($set = FALSE) {

The idea to inject views breadcrumb builder to breadcrumb service
Probably better to implement this in \Drupal\views\Plugin\views\display\PathPluginBase

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.php
@@ -1724,10 +1724,10 @@ public function getBreadcrumb($set = FALSE) {
-        if ($base && $current_breadcrumbs = drupal_set_breadcrumb()) {
+        if ($base && $current_breadcrumbs = Drupal::service('views.breadcrumb')->getBreadcrumb()) {
...
-        drupal_set_breadcrumb($breadcrumb);
+        Drupal::service('views.breadcrumb')->setBreadcrumb($breadcrumb);

This should be a kind of

$views_breadcrumb_builder = \Drupal::service('views.breadcrumb');
$views_breadcrumb_builder->setBreadcrumb($breadcrumb);
// @todo Make priority configurable. ref to feature issue
$priority = 1001;
\Drupal::service('breadcrumb')->addBuilder($views_breadcrumb_builder, $priority);

The priorities needs separate issue to revisit before release because services would be fired in the order:

view - 1001
legacy - 500
menu_link - 0

So if no breadcrumbs would be returned by view then legacy (remove) or menu link should build something

Anyway the breadcrumbs service used only in template_preprocess_page():

  // Set the breadcrumb last, so as to increase the chance of being able to
  // re-use the cache of an already retrieved menu containing the active link
  // for the current page.
  // @see menu_tree_page_data()
  $variables['breadcrumb'] = array(
    '#theme' => 'breadcrumb',
    '#breadcrumb' => \Drupal::service('breadcrumb')->build(\Drupal::request()->attributes->all()),
  );

And in \Drupal\system\Plugin\Block\SystemBreadcrumbBlock

  public function build() {
    $breadcrumb_manager = \Drupal::service('breadcrumb');
    $request = \Drupal::service('request');
    $breadcrumb = $breadcrumb_manager->build($request->attributes->all());
    if (!empty($breadcrumb)) {
      // $breadcrumb is expected to be an array of rendered breadcrumb links.
      return array(
        '#theme' => 'breadcrumb',
        '#breadcrumb' => $breadcrumb,
      );

Update

Added related to summary
#2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.
#2074177: Allow breadcrumb builders to be chained or alter other results rather than forced to run singly

#5: convert_breadcrumbs-2061913-5.patch queued for re-testing.

Here is the start.

Yeah this is not enough yet, there are other references still, especially in the taxonomy part.

updated related

This only one left here so I closed meta as duplicate of this

@dawehner so we should back to #5 approach?

@dawehner so we should back to #5 approach?

Yes, the patch from jibran in #22 is not enough though I still think that the patch in #5 is fundamentally wrong in the sense
that we no longer need a special breadcrumb system just for views. Core itself is more flexible. For example the taxonomy specialized breadcrumb builder now works in a way that it can support both the normal taxonomy page as well as the view page.

ok, let's go extreme.

Nothing is left anymore, nice!!

Favourite kind of patch.

Committed/pushed to 8.x, thanks!

So this didn't actually remove LegacyBreadcrumbBuilder but the parent issue was marked as a dupe of this one?! :-) Was that forgotten here or are there other blockers?

@tstoeckler, #2145463: Remove LegacyBreadcrumbBuilder. I just saw 'in Views module' and excised everything from there.

Right.

Nevermind, #2145463: Remove LegacyBreadcrumbBuilder has it

Let's use the other issue and write the change request here.

Good idea, I like things nicely separated. Then we get better commit log, and better change notice issue refs.

So, fixed? or do we want a views change notice for the UI stuff?

Here is the original change notice for meta Extensible breadcrumb system added. So i don't think it needs new one.

Well, we do remove the API from views, so we have to document that.

ok moving to the views queue then.

What to do with issues like that... close or not?

For more information as to why this issue was moved to the Drupal core project, please see issue #3030347: Plan to clean process issue queue

This issue is about to create change record for already removed code

The remaining task here is to add a change record for an API removed in Drupal 8.x, before Drupal 8.0 was released. Since Drupal 8 is now end of life there is no need to make a change record.

Even though part of this was not finished, marking this outdated isn't right because a patch was committed here. I am going to set the status to Fixed.

Remove tag,