Wrong breadcrumb due to route alterations

Here is a patch.

#2 patch work fine on Drupal Core 9.5.11.

xurizaemon → credited Nasreddine123 → .

xurizaemon → credited nicrodgers → .

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

xurizaemon → credited annoul4 → .

xurizaemon → credited joaopauloscho → .

xurizaemon → credited linhnm → .

xurizaemon → credited mike.vindicate → .

xurizaemon → credited webflo → .

Over in [#] there's a suggestion from @webflo @joaopauloscho to use !array_diff_assoc($routeParams, $rawParams) instead of $routeParams == $rawParams.

$routeParams == $rawParams ensures the keys and values match and doesn't care about order.

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

xurizaemon → closed merge request !17

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.

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.

azovsky → changed the visibility of the branch 3392139-wrong-breadcrumb-due to hidden.

azovsky → changed the visibility of the branch 3392139-wrong-breadcrumb-due to active.

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.

(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 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 between 4cc820aa15b3e0ff2bcb3baa0507912fcd5d7c8a and the new local state.

git diff 4cc820aa15b3e0ff2bcb3baa0507912fcd5d7c8a..HEAD

5. Force-push the rebased changes

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

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

Rebased, back to Needs Review. Thanks all!

Test run has failed with Github timeouts, I see there's an issue 🐛 Random HTTP timeouts for GitLab CI jobs Active 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?

Hi, want to rebase / re-test this now that 🐛 Missing route cachability metadata. Active has landed?

Rebased against 2.0.0 after recent updates ( 🐛 Drupal\menu_breadcrumb\Form\SettingsForm is incompatible with ConfigFormBase Active ).

Thanks @restoismile for input. It's clearer for other collaborators to use the existing merge request rather than add a patch which I then need to compare to the MR. 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... →

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

-+        $raw_parameters = (array) $route_match->getRawParameters();
++        $raw_parameters = (array) $route_match->getRawParameters()->all();

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? That will help this issue to progress more easily.

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

Hi,

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

Have a good day,

Sébastien