Closed indigoxela closed 2 months ago
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:
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.
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.
@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.
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:
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)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:
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. 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.
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).
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.
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.
@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.
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! :pray:
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. :grimacing:
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... :disappointed:
How to reproduce
Order doesn't matter that much - you can also enable the module first, then enable the view. Result is the same - access denied.