As per #1952394-120: Add configuration translation user interface module in core point 21, we should use similar code to https://api.drupal.org/api/drupal/core%21modules%21system%21lib%21Drupal....

CommentFileSizeAuthor
#14 not-empty-route.patch1.52 KBGábor Hojtsy
#11 interdiff.txt2.35 KBGábor Hojtsy
#11 edit-check-access-11.patch12.93 KBGábor Hojtsy
#9 edit-check-access-9.patch13.08 KBGábor Hojtsy
#9 interdiff.txt7.13 KBGábor Hojtsy
#8 edit-check-access-8.patch5.95 KBGábor Hojtsy
#8 interdiff.txt2.42 KBGábor Hojtsy
#6 edit-check-access-6.patch5.48 KBGábor Hojtsy
#6 interdiff.txt4.25 KBGábor Hojtsy
#4 type-alter-hook-4.patch4.84 KBGábor Hojtsy
#4 interdiff.txt768 bytesGábor Hojtsy
#1 edit-check-access.patch4.53 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Active » Needs review
FileSize
edit-check-access.patch4.53 KB

Untested patch.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: +Needs tests

Status: Needs review » Needs work

The last submitted patch, edit-check-access.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt768 bytes
type-alter-hook-4.patch4.84 KB

Wups, forgot to update static create.

Status: Needs review » Needs work

The last submitted patch, type-alter-hook-4.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.25 KB
edit-check-access-6.patch5.48 KB

Inject router, pass on route to be able to find the route for the path.

Status: Needs review » Needs work

The last submitted patch, edit-check-access-6.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.42 KB
edit-check-access-8.patch5.95 KB

It helps if I inject all the needed services. Also pretty ridiculous that I need to inject so much to check access to paths :/

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -Needs tests
FileSize
interdiff.txt7.13 KB
edit-check-access-9.patch13.08 KB

With specific tests for edit access checking. On site information page and contact category translation page, so both regular config and config entities are covered. The later also has dynamic parts on the URI, so tests that this works with params.

Status: Needs review » Needs work

The last submitted patch, edit-check-access-9.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
edit-check-access-11.patch12.93 KB
interdiff.txt2.35 KB

It was failing not because the edit link was showing up but because the edit link is a prefix to the fuller translation links to the xpath matched given its use of contains(@href...). I was thinking about checking more specific links, but then it may match the breadcrumb still :D So with more concrete checking on the Edit link done in situations when there are no translations to edit (only to add), we can more precisely target this situation and properly ensure this link will not be there. Works for me :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Fixed

Committed this one :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Fixed » Needs work 
+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.php
@@ -129,15 +169,24 @@ class ConfigTranslationController extends ControllerBase implements ContainerInj
+        $route_request = $this->getRequestForPath($request, $path);
+        $route_name = $route_request->attributes->get(RouteObjectInterface::ROUTE_NAME);

@@ -180,4 +229,35 @@ class ConfigTranslationController extends ControllerBase implements ContainerInj
+    catch (NotFoundHttpException $e) {
+      return NULL;
+    }
+    catch (ResourceNotFoundException $e) {
+      return NULL;
+    }

I think we either need to bubble up the exception (i.e. not catch it) or do a if (!empty($route_request)) above.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
not-empty-route.patch1.52 KB

Yeah. The method was directly taken from the breadcrumb code, so I'd keep it as close to that as makes sense. Modifying the caller then. (Also theoretically the base route will always be there, since we base the translation tab on the edit path, but you never know :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Fixed

Committed that, thanks for the review :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Thanks!

Status: Fixed » Closed (fixed)

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