Regression in 1.28.x: changed hook order breaks existing functionality

[indigoxela]

Description of the bug

That issue comment by @argiepiano in i18n issue queue explains what happened in core when adding SVG icons: backdrop-contrib/i18n#134 (comment)

As the order of hooks - actually only hook_menu_alter - changed, the whole logic changed a bit. That caused a severe problem in i18n, but might also cause problems elsewhere.

For i18n, for example, it means that taxonomy term pages overridden by views aren't accessible anymore since 1.28.0.

Steps To Reproduce

See the contrib queue issue for an example.

It may be, that this only affects one contrib module, but I actually doubt that. On the other hand I see, why this has been done, so we might eventually not even be able to fix this.

However, it's a potentially breaking change and we should warn people.

Additional information

This is the function: https://github.com/backdrop/backdrop/blob/96a0e03870c9fd09ab4134827a05148173e8e516/core/modules/views/views.module#L552

[indigoxela]

Also note that views by intention "wants" its hooks to run late. See this module weight approach in its install file.

So it seems that this setting of module weight's quite pointless: https://github.com/backdrop/backdrop/blob/1.x/core/modules/views/views.install#L11 Is that correct?

EDIT: it isn't.

[avpaderno]

It is not pointless: module_set_weight('views', 10) does not win over views_module_implements_alter().
Actually, module_set_weight('views', 10) is necessary to set when views_module_implements_alter() is invoked. If the idea is to make all the Views hooks the "last" invoked hooks, but views_menu_alter() must still be the first hook_menu_alter(), both those lines are necessary.

[avpaderno]

(the last invoked hooks is relative. I guess 10 has been chosen because all the other Backdrop core modules use a lower weight. If a new Backdrop core module were to set its weight to 100, Views hooks would not be anymore the last invoked hooks.)

[indigoxela]

@kiamlaluno if that assumption is right, I wonder why the order of hooks effectively changed. Id did, obviously.

Edit: the order of hook_menu_alter() only.

Re module_set_weight() - as an addendum: not only core modules, but any module, including contrib and custom.

This is, how the order worked, but something changed, and that has a potential for regressions like the one i18n_taxonomy currently has to deal with. That submodule sets its weight to 5, so until 1.28.0 views hooks reliable ran after i18n_taxonomy hooks, but now views hooks run before i18n_taxonomy hooks. For sure hook_menu_alter() now runs in different order.

[avpaderno]

Yes, module_set_weight() changes the weight of a module respect all the other installed module.
What my comment meant is that 10 has been probably chosen to make Views hooks the last invoked hooks basing on the weight used by the other Backdrop core modules, not by Backdrop core modules and all the existing contributed modules. (The point is that last hooks is relative to other modules; it does not assure Views hooks will always be the last invoked ones, if some precaution is not taken in writing new code or changing existing code.)

[indigoxela]

@kiamlaluno I wonder, how that should help with the reported problem, though. 😉
I really appreciate your analysis of what module_set_weight() does, but that function's not the cause of the problem. It's the addition of views_module_implements_alter() in 1.28.0.

I only mentioned module_set_weight() in a previous comment, to clarify the "late run" of views hooks was by intention. Sorry, if that was misleading.

[avpaderno]

@kiamlaluno I wonder, how that should help with the reported problem, though. 😉

It was a comment for views by intention "wants" its hooks to run late to mean that is not really the intention of the Views module, as the recent change shows.

Contributed modules should not assume that a specific hook implementation is executed before or after another one, which also means that contributed modules should not assume a Backdrop core module will not implement hook_module_implements_alter() because necessary to implement a new feature.

(As a side note, I find that adding new hooks is probably more risky for compatibility with existing modules.)

[argiepiano]

Thanks for opening this, @indigoxela.

I think the rationale for that change was that, since Views is able to create menu routes and menu items, and since it does not have an easy way to provide icons for those, @quicksketch probably moved views_menu_alter() to be executed first so that it'd be easier to provide an icon for those items in subsequent menu_alter hooks.

Changing the execution order of hook_menu_alter hooks probably was a "bold" and dangerous move - this type of things tends to have repercussions beyond core. I wonder if there may be a different, safer, way to provide icons to Views menu items without changing the menu_alter order? Perhaps by modifying the Views UI to accept an icon name? (and perhaps using an icon browser).

And I agree with @indigoxela in the observation that the Views module has a weight of 10 (instead of the default 0) in order to assure that its hooks run toward the end. Only a couple other core modules have higher weights: admin bar, layout, contextual and standard.profile. So, defeating this by modifying this hook order was bound to create issues.

Granted, the breakage of i18n_taxonomy is solved with that if statement. So we could "wait and see" if something else breaks, or perhaps avoid this change in order and find a better way to provide icons for those Views menu items.

[indigoxela]

@kiamlaluno Hm, honestly, I have to contradict at least the "vibe" of your comment. (Maybe that vibe wasn't intentional, though...)

Changing the hook order is quite a change - even for a minor release. We should warn people - be it in the release notes or wherever.

Three developers perfectly familiar with Backdrop scratched their head for quite a while to figure out, why all term pages are broken all of a sudden (in specific setups).
We shouldn't assume, people less familiar with the technical backgrounds are even able to fix the problem.

For the i18n problem, the solution will (very likely) be a new i18n release.
But other hook_menu_alter() implementations might run into a similar problem.

Just stating "you shouldn't have assumed the order, anyway" is IMO not the way to deal with that.

[avpaderno]

@indigoxela Truly, the comment says contributed modules should not assume a Backdrop core module will not implement hook_module_implements_alter(). It is not referring to you nor any other person in the specific. I did not even check who the maintainers for the module mentioned in the issue summary are.