Some change in core 1.28.0 broke i18n_taxonomy in combination with views

[indigoxela]

Saw it by accident in an otherwise unrelated PR check.

But it's reproducible locally.

With disabled i18n_taxonomy the taxonomy_term view works as expected, but as soon as one enables the module, the view only throws "access denied" for all term pages. No matter what settings the term or vocab has.

Major break. 😬

It must be a newer core change, as ~ 4 months ago things still worked. Now it's broken.

Last successful test run: 2024-03-03.

There have been lots of commits to core since then... 😞

How to reproduce

1. Set up a multilingual site
2. Enable the taxonomy_term view and verify things still work
3. Enable i18n_taxonomy
4. Visit any term page -> access denied

Order doesn't matter that much - you can also enable the module first, then enable the view. Result is the same - access denied.

[indigoxela]

Last successful run - everything was fine: https://github.com/backdrop-contrib/i18n/actions/runs/8130283390

Failing run today: https://github.com/backdrop-contrib/i18n/actions/runs/9873741876

But the test failure in Taxonomy translation (I18nTaxonomyTestCase) is reproducible without that change.

Further narrowing it down:

• It worked up until 1.27.3 (including the security fix from last week - unrelated)
• It is broken since 1.28.0-preview (May 2)

[indigoxela]

This weird approach raises more questions than it answers: #135 (EDIT: this comment is outdated)

Why/how did this ever work, anyway?
Or does this break anything else? At least automated tests work again.

BUT this isn't the solution. Any view can be used to override the path taxonomy/term/%taxonomy_term, not only the one provided by core.
So the recherche has to go on.

[herbdool]

Ah, this rings a bell. We were debugging this exact issue and ended up just disabling the default taxonomy view, since it wasn't really needed in this particular case. I'm going to test this.

[herbdool]

@indigoxela maybe it just needs to be:

if (!module_exists('taxonomy_display') && isset($items['taxonomy/term/%taxonomy_term'])) {

It is only trying to override that one route. And if a view is providing a taxonomy term route, the route would end in %view_arg in hook_menu from what I can tell. So this would maybe account for any view overriding the taxonomy page.

[argiepiano]

I've dug a bit, and this is what I'm seeing:

First a bit of background. The way Views overrides a menu route is by basically "hijacking" any menu route definitions created by other modules - in this case taxonomy. It actually unsets the item in views_menu_alter. So, in order for this to work, it must run after other implementations of hook_menu_alter() that may have altered other routes.

So:

• In version 1.27.x, i18n_taxonomy_menu_alter runs before views_menu_alter. 👍🏽 . Meaning that what i18n_taxonomy_menu_alter() does to the taxonomy/term/%taxonomy_term is actually lost completely when Views hijacks the route (in fact this probably means that the functionality provided by i18n_taxonomy was lost when the View was enabled)
• In version 1.28.x, for reasons still unknown to me, the order of the hooks is changed dramatically. views_menu_alter() now runs first, unsetting taxonomy/term/%taxonomy_term. Then i18n_taxonomy_menu_alter() runs, and sets anew some stuff for that route, EXCEPT the access callback, since it assumes that the access callback was provided by taxonomy.

We have two problems:

1. the Access denied, which could be fixed the way @indigoxela did in this PR. In fact, with 1.27.x, AFAIK, i18n_taxonomy probably didn't do anything if the View was enabled. So, the PR basically avoids the Access denied, but the non-functionality of i18n_taxonomy remains the same as before when the View was enabled.
2. The bigger problem is: why has the hook_menu_alter execution order been modified so dramatically? Will this cause other issues, since now views_menu_alter() is running so very early? It may.

I'll try to figure out an answer for the first part of number 2 above.

[argiepiano]

OK, I've found what causing views_menu_alter() to run first. It was part of the PR to add the SGV icon system in 1.28. There is a new implementation of hook_module_implements_alter() in views.module which moves the hook to the top, so that other modules can alter it (?). Not sure what the rationale was for that (I tried to figure that out in the issue, but there are tons of comments).

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

Soooo... not sure we can do anything about this. Hopefully that won't cause other issues with contrib.

So, the PR here is fine: it allows the user to access the View.

[argiepiano]

One suggestion, though, for the PR. Instead of checking whether the (hard-coded) View is enabled, I would check if the route taxonomy/term/%taxonomy_term is set. If it is not, it means that it was already hijacked by the View.

[indigoxela]

@argiepiano wow, great sleuthing! And an alarming finding IMO. Changing the order of hooks is almost guaranteed to break something in contrib or custom.

I can confirm that the item $items['taxonomy/term/%taxonomy_term'] is NULL when the view is enabled, but an array when it's not. (An array in Backdrop 1.27.x)

PR updated.

[indigoxela]

Since this is a severe problem, I went ahead and merged, next step will be a release.

Special thanks to both of you, @argiepiano and @herbdool for your help! 🙏