Wrong breadcrumb due to route alterations

#2 patch work fine on Drupal Core 9.5.11.

I pulled @nicrodgers sort before comparison of the route and raw parameters over from #3395684: Current page added twice due to route alterations .

Over in #3362822: Duplicate the current page of views there's a suggestion from @webflo @joaopauloscho to use !array_diff_assoc($routeParams, $rawParams) instead of $routeParams == $rawParams.

Neither method cares about order, however:

• $routeParams == $rawParams ensures the keys and values match.
• !array_diff_assoc($routeParams, $rawParams) ensures that all entries in $routeParams are matched in $rawParams (and allows for additional $rawParams entries).

php > $routeParams = ['b' => 'b', 'a' => 'a'];
php > $rawParams = ['a' => 'a', 'b' => 'b'];

php > var_dump($routeParams == $rawParams);
bool(true)

php > var_dump(!array_diff_assoc($routeParams, $rawParams));
bool(true)

php > $rawParams['c'] = 'c';
# All values in routeParams exist in rawParams.
php > var_dump(!array_diff_assoc($routeParams, $rawParams));
bool(true)
# Some values in rawParams exist in routeParams.
php > var_dump(!array_diff_assoc($rawParams, $routeParams));
bool(false)

Which comparison is more correct here?

Seems this can be repro'd as easily as taxonomy/term/1 - debug here from \Drupal\menu_breadcrumb\MenuBasedBreadcrumbBuilder::addMissingCurrentPage() shows that route and raw parameters may be sorted differently. Added test \Drupal\Tests\menu_breadcrumb\Functional\MenuBreadcrumbTest to cover this.

how can I test? I have the same problem

@zenimagine, here are docs for patching modules when using composer

Should I apply patch #2 ?

Moving this back from RTBC since there are changes to be reviewed!

@zenimagine, on this issue a Merge Request ("MR") has superseded the patch submitted in #2. So review should focus on the changes available from the MR. On the MR URL, you'll see tabs for eg "Commits" (the initial change from #2, and later changes) and "Changes" (the current proposed changes from this MR).

The required patch is linked on this issue (text "plain diff" near MR !15), which is the MR URL with .diff appended.

("diff" and "patch" files are approximately the same thing, they both are files that you can use with patch utility.)

That file can be saved and used as a patch in your local codebase, or (quicker but not recommended for production builds) referenced directly in your composer patches configuration.

Patch #2 applies to version 2.0.0-alpha0, but the MR-15 does not. I'm guessing that the Merge Request only applies to the 2.0.x-dev version

MR15 no longer applies to 2.0.x-dev. I think it needs rebasing.

Hi, I was just trying to update the merge request, I've probably done something wrong - https://git.drupalcode.org/issue/menu_breadcrumb-3392139/-/pipelines/310...

Sorry.

Hi @matason, I'm interpreting your comments as indicating your last few commits should be reverted.

I will do that, and your changes will be preserved at https://git.drupalcode.org/project/menu_breadcrumb/-/merge_requests/15/d... - the history of pushes to the MR is visible at https://git.drupalcode.org/project/menu_breadcrumb/-/merge_requests/15/p... should you need to restore something.

If I misinterpreted your intent, you are welcome to restore proposed changes to the MR :)

(The force push in 5 below will overwrite the state of https://git.drupalcode.org/project/menu_breadcrumb/-/merge_requests/15/c..., but the pipelines tab will continue to record pushes of commits that are now removed.)

The commands I used to do this are:

1. Check out the branch from this MR:

git remote add menu_breadcrumb-3392139 https://git.drupalcode.org/issue/menu_breadcrumb-3392139.git
git fetch menu_breadcrumb-3392139

2. Obtain the last commit which we want to preserve

Sourced from https://git.drupalcode.org/project/menu_breadcrumb/-/merge_requests/15/c... (I used 4cc820aa15b3e0ff2bcb3baa0507912fcd5d7c8a)

3. Rebase the local changes against upstream target branch.

git rebase origin/2.0.x

4. Confirm that there are changes

Compare between 4cc820aa15b3e0ff2bcb3baa0507912fcd5d7c8a and the new local state. We want to see there are some changes rebased from the upstream here.

git diff 4cc820aa15b3e0ff2bcb3baa0507912fcd5d7c8a..HEAD

5. Force-push the rebased changes

git push -f menu_breadcrumb-3392139 3392139-wrong-breadcrumb-due

Rebased, back to Needs Review. Thanks all!

Test run has failed with Github timeouts, I see there's an issue #3445532: Random HTTP timeouts for GitLab CI jobs for that mentioned in Slack recently. Will retry once then leave it.

Thanks for sorting that out, xurizaemon!

Best,

Chris

I found that MR26 doesn't apply if you're using the Drupal 11 compatibility patch in https://www.drupal.org/node/3485520 due to them both modifying the params in MenuBasedBreadcrumbBuilder::construct().

So i've re-rolled the attached patch, which enables you to patch this issue on top of the other one.

If this issue gets committed before the other one, then use the MR.

I've been testing this out, and the breadcrumbs are still duplicated on views pages. It looks like this is because the fix has been applied within the taxonomy term logic so for views pages it never gets hit and the default route params are still missing. Was there a reason this had to be nested inside the taxonomy term logic rather than being outside so it can apply everywhere?

Steps to reproduce

• Add a view and configure it to have a menu item link.
• Go to that page
• Observe the page appears twice in the breadcrumb trail

Hi, want to rebase / re-test this now that #3495164: Missing route cachability metadata has landed?

Rebased against 2.0.0 after recent updates (#3485520: Drupal\menu_breadcrumb\Form\SettingsForm is incompatible with ConfigFormBase).

Correction of $raw_parameters, it was incorrectly recovered.

Thanks @restoismile for review and apologise to all if I missed that detail when rebasing.

@restoismile it's clearer for other collaborators to use the existing merge request (see "get push access" / "show commands" near top of issue) rather than add a patch which then needs to be compared with / considered alongside an existing MR (merge request). Drupal's Gitlab process allows you to add your own changes, and you'll then receive credit more properly too.

- https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

If the issue already has a MR open, that should be preferred for collaboration.

I believe the additional change you're proposing (restoring!) is in src/MenuBasedBreadcrumbBuilder.php:

   MR: $raw_parameters = (array) $route_match->getRawParameters();
patch: $raw_parameters = (array) $route_match->getRawParameters()->all();

As \Symfony\Component\HttpFoundation\ParameterBag::all returns an array, I suspect the (array) there is redundant and can be removed if we use ->all()?

Would you please take another look and propose the change via the MR, considering the above when doing so? That will help this issue to progress more easily.

Hi,

I just pushed the fix of @restoismile to the MR.

Have a good day,

Sébastien

Patch from #39 does not apply anymore, adding a new one based on https://git.drupalcode.org/project/menu_breadcrumb/-/merge_requests/15/d....