Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The taxonomy page is now a view, so visiting /taxonomy/term/X will use a view to display the list. This view may be modified to use optional arguments as it was before #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views where the view used a term_node_tid_depth argument instead of a single tid argument. That modifies the path under the route. However content translation is not ready to fill in route parameters other than the default entity identifier expected, so such modifications in the path under the route result in fatal errors from content translation:
Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "arg_1" for route "/taxonomy/term/{taxonomy_term}/{arg_1}/translations" must match "[^/]++" ("" given) to generate a corresponding URL. in Symfony\Component\Routing\Generator\UrlGenerator->doGenerate() (line 167 of core/vendor/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php).
Proposed resolution
We should inherit route parameters in the content translation routes from the canonical route for this use case.
Remaining tasks
Devise fix
Write tests
Reviews
User interface changes
None.
API changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 2324809-test-only.patch | 3.94 KB | jibran |
| #5 | 2324809-try-default-arg1.patch | 1.52 KB | jhodgdon |
Comments
Comment #1
dawehnerAdding a kind of related issue
Comment #2
Gábor HojtsyThis seems to be all down to ContentTranslationRouteSubscriber only assuming the entity_type_id (in this case taxonomy_term) parameter on the route instead of copying over all the route parameters from the base route. Theoretically that should not be too hard to resolve (famous last words).
Comment #3
jhodgdonWhy does it only affect the view and not the default term listing?
Comment #4
Gábor HojtsyThe default route for taxonomy terms is (from taxonomy.routing.yml):
On the other hand, the view has a depth argument which is added on the path (but has a default value, so is optional):
/taxonomy/term/{taxonomy_term}/{arg_1}(Quoted from your error message, generated in
core/modules/views/src/Plugin/views/display/PathPluginBase.php) See also the path pattern for the feed attached to the taxonomy term view (incore/modules/taxonomy/config/install/views.view.taxonomy_term.yml):path: taxonomy/term/%/%/feedViews is relying on a trick here that now the same route can be replaced with a different route that may have more arguments. The tabs are route-associated, so we can still put a tab that is not technically on the same path level even if the taxonomy term page uses a depth argument, we just need to inherit the route arguments in the ContentTranslationRouteSubscriber instead of assuming only the entity type one exists.
Comment #5
jhodgdonAh, OK.
So to fix it, you're saying that in ContentTranslationRouteSubscriber where it is setting up the
new Route(), we just need to add some more to the defaults array if there are additional arguments? That seems like an easy fix. Let's see...I tried this (see attached patch). It does add 'arg_1' to the defaults... but the same error still occurs, and actually I think the path for translations shouldn't have the /{arg_1} in it, right? I don't think we want the path to use for translations to change, just because we've enabled this view, do we?
Comment #6
jhodgdonFixing title...
Comment #7
Gábor Hojtsy@jhodgdon: Yeah that is exactly what I had in mind.
As for how we should tie to the original path, we ask the entity type for the canonical route and this is what we get. How else can we get the non-views path if the route given by the entity info is already altered? (I believe views alters the route but leaves the entity info alone).
Comment #8
jhodgdonI have no idea how to get the non-altered route.
So just to be clear, that patch I uploaded doesn't actually work -- the Taxonomy view still has a fatal error with Content Translation enabled. I just uploaded it so you could see what I was trying to do. It accomplishes the goal of adding the default to the Route constructor, but that doesn't fix the bug.
We should probably write a test for this too -- steps to reproduce are in the issue summary.
Comment #9
jibranWriting some test.
Comment #10
jibranThanks for the steps @jhodgdon. This is test only patch.
Comment #12
jibranSee #1857256-165: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views fixes this bug. I am going to add these test there.
Comment #13
Gábor HojtsyI see #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views resolves the fatal from by removing the depth argument currently in the view (but that is not THE reason to remove the depth argument, so yay for yet another good side effect), but this issue is not resolved for the case when someone does want to do that or override other entity pages with views that may have more dynamic arguments. So I think this is still worth reopening on a lower priority level. Updated issue summary for that.
Comment #14
dawehnerOH I see, yeah there are several problems here
What could help you here is #2285413: [Meta] Standardize entity route names, let me explain why.
This issue on the long run plans to replace the link templates to be paths again. This means that the routes
for content translation as well as config translation, would rely on that path information. So your generated routes will look like taxonomy/{taxonomy_term}/translate. In case you worry, the routing system always prefer non-placeholders in the path against placeholders.
Comment #15
jhodgdonThe taxonomy listing is now using Views; the original issue here is obsolete due to #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views... Needs issue summary update if this issue is to stay open, and I don't think this is really about D8MI any more is it?
Comment #16
Gábor HojtsyUpdated. It is still a D8MI issue because this would be a problem with the route added by content_translation module on top of a route that Views modified from the original.
Comment #19
andypostComment should tell why they skipped
Comment #29
longwaveDiscussed with @Lendude in #bugsmash triage.
The IS is brief and not entirely clear; what would be helpful here is someone figuring out how to reproduce the issue since the direction changed in #13, and either writing down the steps or exporting the view that causes the bug.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedSince there hasn't been a follow up to #29 going to close out for now.
If still a bug for D10+ please reopen updating issue summary with steps to reproduce
Thanks all!