Service menu.active_trail dependency injection

Apologies for being out of the loop for a couple weeks due to a travel related illness. This module was originally written to use the `@menu.active_trail` service which was only recently reversed (#3026188: Breadcrumbs building does not fully use given RouteMatchInterface). As far as I can tell, your patch above simply reverses the patch for the earlier issue... although leaving in the inline comment from the first patch which explains why it was made.

Therefore we now have a conflict of interest, at least between the two patch providers, and more generally between two groups of users: those who need compatibility with `drupal/menu_position` as described above, and those using dependency injection affecting the `@menu.active_trail` service as described in the earlier issue.

We are going to need consensus from more developers before any future decision to undo #3026188: Breadcrumbs building does not fully use given RouteMatchInterface. Since that change has already been accepted into version 1.8 I am cross-linking these issues, marking this as active since we are not simply waiting to review the provided patch. The other issue will remain closed since the problem described has been fixed, and to avoid confusion all further discussion should take place in this thread.

Maybe someone will come up with a way of reconciling the two modes of behaviour. Though unattractive to me personally, another option might be introduced to toggle the behaviour between an injected & a derived MenuActiveTrail... but even then we would need consensus about which behaviour should be the default.

Hi there, I can already tell what my use case was, and why it was not working without the patch in #3026188.

I wanted to display breadcrumbs for each node entity in a Search API view. Using the request global MenuActiveTrail service, but allowing for a RouteMatch to be passed was causing unexpected breadcrumbs to appear. So I investigated and found out that the MenuActiveTrail service was based on a RouteMatch object, thus it needed to be constructed per given RouteMatch object, and not injected.

I am not really aware of the menu position module behavior and feature, so maybe @siegrist can point out something interesting, a solution that could fit both situations ?

From my point of view, the patch in #3026188 fixed an architectural flaw which can not simply be reverted. We should find a better solution, maybe create an other service.

Patch works for me. I have the same use case; pretty much always use this module with menu_position module as well.

thanks @erichomanchuk - just please new visitors also note that the only behavioural change with version 1.8 was to remove the dependency injection for MenuActiveTrail, so version 1.7 can also be used instead of the patch.

I think comment #4 votes in favour of providing an option to provide either a non-injected OR an injected MenuActiveTrail... if we are getting drop-ins about this issue then other people are noticing the change, after upgrading to 1.8, based on the menu_position use case and looking for open issues.

That also suggests the default of this option should be the same as the module's historical behaviour, i.e. an injected MenuActiveTrail. Users needing to construct the active trail by the RouteMatch passed to the breadcrumb builder can set this option the other way. I would phrase the option something like "Derive MenuActiveTrail from RouteMatch: If FALSE, the injected @menu.active_trail service will be used." and set its default to FALSE.

Since I don't have a test environment with either the menu_position module or @Renrhaf's more complicated use case, would either @Renrhaf or @siegrist be willing to contribute a patch providing this option? In the meantime we could wait for more votes & opinions, but that patch would settle the issue for both sides.

Error on upgrade to 1.7 to 1.8 (#3042459: unexpected error following upgrade to 1.8) suggests argument list to the module should be kept the same as in previous versions. Unless I've read the debugging trace wrong, that error was coming up because the list of injected services had one dropped out, displacing the arguments into different positions: i.e. in the new patch, the @menu.active_trail service should always be passed as a module argument (so it will be there regardless of the option setting to use the derived MenuActiveTrail instead).

If you want to see how quickly they will respond to a ticket on active_trail, check #2820751: Menu Active Trail is empty for built-in view pages which was related to early menu_breadcrumb issue #2820206: No active trail for views with a NULL parameter. We are still waiting for an answer for that one after 2.5 years and finally had to declare the misbehaviour "intended" by core. But in this case the affected user base is too large to ignore, and we would get Drupal 9 before getting an upstream fix.

I know I said hacking in an option was unpleasant, but there is nothing ugly in itself about accommodating two implementations of a flawed design that we cannot fix. Therefore I'll patch in the option myself and then rely on @siegrist and @Renrhaf to test the resulting dev version in their own environments (stay tuned).

OK, please test this in your own environments @siegrist (with new option OFF) and @Renrhaf (with new option ON). A cache rebuild is required & in a contemporary patch I'm also making this more clear in README.txt since it may be impossible to avoid further juggling to the module argument list (though we could have avoided it in #3026188: Breadcrumbs building does not fully use given RouteMatchInterface if I'd been paying more attention).

Testing from others also welcome & this will be released as version 1.9 as soon as enough people give the OK, or in 2 weeks if no faults reported.

dear @Siegrist - yes, the patch is already integrated into the dev release (comment #9 above). That patch file is a git diff between the current dev and the dev without the most recent commit (which shows the same as the patch file).

Were you expecting that this patch was relative to 1.8-stable? You filed this issue relative to 8.x-1.x-dev so I thought that would be the reference version for the patch. In any case please let me know what I may have missed.

Okay! Then we are just relying on @Renrhaf to test the other use case, since the current code in dev has NO duplicate definition of the class property $menuActiveTrail (please check the hash for this commit matches the short one in comment #9 above): https://cgit.drupalcode.org/menu_breadcrumb/tree/src/MenuBasedBreadcrumb...

The patch file attached to this issue has no fundamental difference from the patch or diff files here: https://cgit.drupalcode.org/menu_breadcrumb/commit/?id=364e3f9

I've just done a composer install of a new site & added + enabled the dev module version (composer require drupal/menu_breadcrumb:1.x-dev) and it works fine with no errors from the module. I've downloaded the dev tarball from the front page & it shows no duplicate code definition. So @Siegrist after ruling out all other possibilities I have to conclude that you are applying the patch incorrectly and/or have some artefact in your code environment that is causing the duplicate definition.

You should also be aware from the comments above that the definition of $menuActiveTrail was removed in a recent commit and then added again in the commit above. If you follow that lead I think you will find what you are doing wrong, but there is definitely no such duplicated code in the versions that the general public is using.

So far no additional objections so FYI everybody I am going to release the current dev, with this commit, into version 1.9 on 10 April (2 weeks after that commit) unless there are any additional problems identified. I am still hoping to hear a positive report from any dev users with the "derived active menu trail" option set, à la comment #3 above.

Hi rphair, thanks for the fix ! I tried it for my particular situation and with the "Derive MenuActiveTrail from RouteMatch" checked everything works properly. Without it, it does not work as intended. So for me it's a green flag to go :)

dear @Renrhaf that makes all the difference, since by default everyone else on the dev version is testing the other condition of that option. I'll still commit this on the 10th since I'll have time those few days to fix anything if it goes wrong. Thanks for checking in with your results.

Released from dev into version 1.9.