Problem/Motivation

Content translation inherits admin page status for translation pages from the edit page of entities. If the NodeAdminRouteSubscriber or AdminRouteSubscriber run after the content translation route subscriber though, this inheritance will not work. The ContentTranslationRouteSubscriber should run as late as possible.

Proposed resolution

Set priorities on ContentTranslationRouteSubscriber, NodeAdminRouteSubscriber and AdminRouteSubscriber relative to each other, so content translation goes last.

Remaining tasks

Tests, review.

User interface changes

Admin route inheritance will work proper.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

For example, try custom menu links.

YesCT’s picture

this was fixed in part 4 #2301317: MenuLinkNG part4: Conversion
with

diff --git a/core/modules/menu_link_content/menu_link_content.routing.yml b/core/modules/menu_link_content/menu_link_content.routing.yml
index f96b22d..cebc134 100644
--- a/core/modules/menu_link_content/menu_link_content.routing.yml
+++ b/core/modules/menu_link_content/menu_link_content.routing.yml
@@ -11,6 +11,8 @@ menu_link_content.link_edit:
   defaults:
     _entity_form: 'menu_link_content.default'
     _title: 'Edit menu link'
+  options:
+    _admin_route: TRUE
   requirements:
     _entity_access: 'menu_link_content.update'
YesCT’s picture

dawehner’s picture

This was actually the wrong fix as it fixed the symptoms, but not the reason.

YesCT’s picture

ok. when we fix it the correct way, we should check to see if the similar fix for nodes in 2276387 can be undone also.

YesCT’s picture

Issue tags: +Needs tests
Gábor Hojtsy’s picture

Issue tags: +language-content

Shortcuts have the same problem, see #2320037: Non-fieldable entities (with only base fields) cannot be configured translatable, eg. shortcuts which includes the same workaround that is in menu_link_content.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +sprint
FileSize
2.47 KB

Does any of the existing ones have tests, besides node translation pages?

Gábor Hojtsy’s picture

Title: admin themes for translation tabs just works for nodes » Admin theme for translation tabs just works for nodes
Category: Task » Bug report
Gábor Hojtsy’s picture

Duh, not going to work. Instead of service priorities, we should set event priorities. This works.

Gábor Hojtsy’s picture

Now with tests. The interdiff is the TEST ONLY patch. It should not fail since we have the hardcoded admin page designation in the route. Once we remove that, but without fixing the priorities, it will not work (see TEST ONLY WILL FAIL patch). The fix and test overall is in the 11 patch.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/content_translation/src/Routing/ContentTranslationRouteSubscriber.php
@@ -168,7 +168,9 @@ protected function alterRoutes(RouteCollection $collection) {
+    // Should run after AdminRouteSubscriber so the routes can inherit admin
+    // status of the edit routes on entities. Therefore priority -201.
+    $events[RoutingEvents::ALTER] = array('onAlterRoutes', -201);

How about -210 so there is a gap between AdminRouteSubscriber and ContentTranslationRouteSubscriber?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
3.96 KB

Sure thing. The only side effect is that requires the param converter route subscriber to be changed as well since otherwise they are the same priority and the params are not yet converted when access tries to deal with them (content translation comes before paramconverter in the alphabet).

Gábor Hojtsy’s picture

Better said, the content translation routes would not have param converters applied properly if the content translation routes are added after the param converter altering is done. (This shows in severe test fails if we don't change the param converter priority).

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 15: 2310475-route-subscriber-event-priorities-15.patch, failed testing.

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Was random test fail.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3254a1a and pushed to 8.0.x. Thanks!

  • alexpott committed 3254a1a on 8.0.x
    Issue #2310475 by Gábor Hojtsy | dawehner: Fixed Admin theme for...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

dawehner’s picture

Great work!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.