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
ContentTranslationRouteSubscriber sets the _admin_route option for translation routes based on the respective edit route's setting. However, it sets _admin_route to FALSE even if the edit route does not specify _admin_route at all. This is not only sloppy but opens the door to a very subtle bug: If a route subscriber runs after ContentTranslationRouteSubscriber it thinks that the translation routes have explicitly disabled _admin_route (like the block demo page does), even though they haven't.
Proposed resolution
Make ContentTranslationRouteSubscriber precisely reproduce the _admin_route option of the edit route, i.e. if it is not set, do not set it either.
Remaining tasks
Is this change still needed?
User interface changes
None.
API changes
Only the priority of ContentTranslationRouteSubscriber is changed, so only modules interacting specifically with content translation routes (I don't know any) will notice this. And since it is only moved earlier in the chain even those module won't break in the majority of cases.
On the other hand for modules interacting with the _admin_route option this might fix a very edge-case-y bug, they probably didn't they had. :-)
Beta phase evaluation
| Issue category | Bug because _admin_route is not set correctly |
|---|---|
| Issue priority | Normal |
| Unfrozen changes | Not unfrozen |
| Prioritized changes | Prioritized because it is a bug fix |
| Disruption | This is technically speaking an API change but the actual disruption is zero or very, very close to it |
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2415205-4.patch | 4.7 KB | tstoeckler |
Comments
Comment #1
tstoecklerHere we go.
Comment #2
dawehnerThis is a much better solution for the problem.
Can we come up with some test coverage for some specific examples?
Comment #4
tstoecklerHere we go.
The problem was that $admin_route was being conditionally set in a foreach-loop but never being reset.
Comment #5
tstoecklerRegarding test coverage: We implicitly test both
!$route->hasOption('_admin_route')(CommentTranslationUITest) and$route->getOption('_admin_route') === TRUE(MenuLinkContentUITest).What we do not test is
$route->getOption('_admin_route') === FALSE, i.e. have an entity type whose edit form lives under/adminbut (like the blocks demo page) does not use the admin theme.In theory, explicit tests would be preferable over implicit tests, but given that an entire entity type needs to be created for each test case, I'm not super excited that.
So holding off on more test coverage for now, but ...
Comment #6
Gábor HojtsyComment #7
Gábor HojtsyFor test coverage I was looking if you can just add a route subscriber to remove the admin route option, but looks like hasOption() checks for the array keys and there is no unsetOption() or removeOption(). Also the setOptions() does not override existings options, so it does not seem to be useful to unset options. Should routes support that somehow at least for testing?
Comment #8
tstoeckler@Gábor Hojtsy: Ahh yes,
Route::getOptions()doesarray_key_exists(). That sucks, I wasn't aware of that. Will open a Symfony PR for aremoveOption()if I get to it.You're right though, that we could use #2350503: Add route generation handlers for entities to re-use an existing test entity type instead of providing a bunch of new ones. That's a great idea, thanks!
Comment #9
Gábor HojtsyComment #10
Gábor HojtsyNot being worked on actively, removing from sprint.
Comment #11
kerby70 CreditAttribution: kerby70 commentedCorrecting non standard tag 'Need tests' to 'Needs tests'
Comment #24
quietone CreditAttribution: quietone at PreviousNext commentedThis was an issue in a bugsmash group triage meeting. I was the only who looked at this and I don't know routing but I see that the proposed changes in the patch are not in core. I am going to set this to get more information to confirm that this is still and issue and that this should be pursued.
If this issue still relevant?