Move menu_link "active" class logic into a preprocess function and base it on "active-trail"

+1

No patch or further complaints about it.

Ok. I re-read the issue(s). I'm still a little unclear as to what the "bug" is exactly.

The problem with using the "active-trail" bit is that it isn't always that accurate either in 7.x; especially if the link's href if <front>.

Also, whether this code is in bootstrap_menu_link() or bootstrap_preprocess_menu_link() doesn't really matter. They're both invoked after modules.

That being said, I'll upload a patch here shortly to see if it helps with what you're talking about.

Actually, no need for an elseif.

The 'active' class isn't, however, and should thus be added in a preprocess function so other modules/subthemes can at least remove it again.

1. Yes, it is specific to this base theme. Almost every Bootstrap nav based component requires the an "active" class is present on the <li> element. That isn't a requirement in core, hence why this is added.
2. Modules cannot/should not, ever, remove something that was added by any theme. That is not how the system is designed to work.
3. Yes, sub-theme's would have to override this theme function. That is how the system is designed to work.
4. The theme function isn't big enough to justify what you're proposing. This is for a seemingly small use-case (of which, no use case has yet to be provided) for why this is is actually needed.
5. In 8.x-3.x, this "active" class is added on inside the Twig template itself (the theme function equivalent).
6. Ultimately, moving this to a preprocess would technically be a BC break. A change notice would need to be created. This is not something I'm not too happy about introducing.

I'm trying to compromise here, but you're not understanding what the complexities your proposal actually entails.

The patch in #7 is, more or less, the best I can come up with. Have you tried it?

$element['#href'] == '<front>' && drupal_is_front_page()

Like I said above, there's no other way to get the "active" class on these links. It doesn't get the active-trail class or even the in_active_trail flag set on the original menu item. Not even with a helper module like Menu Trail by Path does this work. This is primarily due to the broken nature of core in 7.x and why in 8.x the entire menu system was replaced with the concept of true routes. This logic cannot be removed. Period.

Because we effectively used that menu as a "space switcher", we needed to unset the active class on the link that was not pointing to the active space.

Honestly though, this sounds more like a fundamental architectural flaw in the decisions that were made. Using a not so widely installed (< 3k) contrib module in an attempt to provide another alternative way to solve another type of multi-site scenario. This project simply cannot cater to every single contrib/custom module that decides to do things just a little differently for their use cases. I could understand more if it was something that is a commonly used module/best practice.

This was impossible without overwriting the theme function, for something that ideally could have been done in a preprocess function.

I'm trying to understand here. How is copying and pasting < 40 lines of code is too difficult? I honestly cannot understand the rational behind this argument. If it were, say, regarding something extremely more complex like the page theme hook.. then sure, this would make sense to do. As it stands now, it literally can be replaced in a sub-theme within 5 seconds.

I still don't see how you consider the active class as something Bootstrap-specific. If you look at menu_tree_output(), menu_navigation_links()...

Yes, these functions do add an active class, but on the link itself (#localized_options)... not on the <li> element wrapping it. That is what I meant by that it is unique to what Bootstrap expects for their nav components to function properly. The active class that is set on the links themselves have absolutely no effect in Bootstrap.

I don't mean to annoy you with this, even if I inadvertently have.

Not at all, you're perfectly fine :D A common misconception when people interact with me is that, while I may be annoyed, it's not at them personally. It has more to do with the fact that little to no information was provided to describe the issue at hand, provide steps to reproduce and justification through use cases.

As you can see here, it's taken several comments to get to a point where I actually understand what's going on here (in your situation). I have limited time and that's why the issue was closed after 8 months of... nothing. No further explanation other than "#1896674 introduced broken logic". That doesn't give me a lot to go on when no one else has really complained about this.

I'm not unsympathetic to your issue, I'm just burdened by responsibility to all for the quality and integrity of this code.

---
I still don't necessarily agree the above rational behind moving this into a preprocess function. That being said, this issue will still need a change record regardless to inform people that the logic for this has changed and if they overrode the theme function in their sub-theme, they will need to update it.

Since we're going to have to do that anyway, I suppose we can also go ahead with the move to a preprocess function. The change record can just say that they should remove the logic for this in their overridden theme function and let the base theme handle it. If they did indeed customize this active class logic, then they will need to move that into a sub-theme preprocess function.

I'll commit a fix for all of this shortly.

Cautionary note: I may still, ultimately, revert this commit/decision if enough people complain about BC breaks. Menus are already a touchy subject in this project and many already customize it to suit their needs. I won't be all that surprised if this causes a lot of grief.

Change record: https://www.drupal.org/node/2778733