Per #1793520-118: Add access control mechanism for new router system, we want a couple routes converted to the new Route system in order for the DX of the access control integration proposed in that issue to be evaluated. This issue leaves todos for the other issue to solve.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, user-route.patch, failed testing.

tnightingale’s picture

Heh requests within requests anyone? http://skitch.affinity01.cantrust.org/Access_denied_%7C_wscci.local-2012...
Manually testing Drupal\user\Tests\UserRegistrationTest in UserRegistrationTest.php:37
Will be sprinting on wscci in Vancouver tomorrow morning, plan to tackle this if no-one's gotten to it before me :)

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.5 KB

Thanks for the offer, but I just got this finished (I think). Amazing what kinds of things you can uncover in the new routing system by converting a real route.

Status: Needs review » Needs work

The last submitted patch, user-route-3.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
7.69 KB
+++ b/core/lib/Drupal/Core/Routing/PathMatcher.php
@@ -56,7 +56,7 @@ public function __construct(Connection $connection, $table = 'router') {
-    $path = rtrim($request->getPathInfo(), '/');
+    $path = '/' . $request->attributes->get('system_path');

This fixes PathMatcherTest accordingly, but probably in a way Crell won't like. Any thoughts on the right way to adjust tests? The reason for this change is to handle language prefixes (e.g., when getPathInfo() is /fr/user/register, we need to match on /user/register) and path aliases.

Crell’s picture

Ugh, that nonsense again.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php
@@ -88,6 +88,7 @@ function testExactPathMatch() {
     $request = Request::create($path, 'GET');
+    $request->attributes->set('system_path', trim($path, '/'));

lolwut? Why do we need that? system_path should have the language prefix stripped out already by the path listeners. If not, then there's a bug in that code.

+++ b/core/modules/user/lib/Drupal/user/UserRouteController.php
@@ -0,0 +1,31 @@
+/**
+ * Returns responses for User module routes.
+ */
+class UserRouteController {

Even if we're not using it yet, we should probably go ahead and make this class ContainerAware. That at least exposes the container to it.

effulgentsia’s picture

system_path should have the language prefix stripped out already by the path listeners

It does. $request->getPathInfo() doesn't, and as I understand it, shouldn't.

Crell’s picture

Right, getPathInfo() is the raw request URI, which is why we should rarely if ever be using that.

effulgentsia’s picture

So does that mean:

    $request = Request::create($path, 'GET');
    $request->attributes->set('system_path', trim($path, '/'));

is the correct way to prepare $request for the PathMatcher unit tests?

Even if we're not using it yet, we should probably go ahead and make this class ContainerAware.

Can we defer that to a follow up? I'm not too keen on adding a setContainer() implementation on every controller class. I'd like to at least have a discussion about creating a base class along the lines of Symfony\Bundle\FrameworkBundle\Controller\Controller if our plan is to make all controllers be container aware.

Crell’s picture

For now since the class isn't extending anything else you can just extend ContainerAware and call it a day. But it's not a blocker for this patch if you don't.

Aw snap! The PathMatcherTest tests don't account for path aliasing and such, which means, duh, we're doing the wrong thing by checking getPathInfo() in PathMatcher. Crell--.

So that means we either do something more or less like what #5 is doing (although possibly calling getPathInfo() rather than setting $path explicitly), or we adjust PathMatcher to first check for system_path, and if it's not found fallback to getPathInfo(). The latter sounds more robust to me.

effulgentsia’s picture

FileSize
4.41 KB
7.4 KB

Ok, how's this?

Crell’s picture

+++ b/core/lib/Drupal/Core/HtmlPageController.php
@@ -62,8 +62,13 @@ public function content(Request $request, $_content) {
-    $page_content = $response->getContent();
+    // For successful (HTTP status 200) responses, decorate with blocks.
+    // @todo Are there any other statuses for which we should also do this?
+    if ($response->isOk()) {
+      $page_content = $response->getContent();
+      $response = new Response(drupal_render_page($page_content));
+    }
 
-    return new Response(drupal_render_page($page_content));
+    return $response;

I can't think of others off hand. A 403, 404, 405, or 415 would all get turned into a full page response by the response listener anyway, so no need to handle that here. The question is whether this code would cause problems for a 204 or similar.

+++ b/core/modules/user/lib/Drupal/user/UserRouteController.php
@@ -0,0 +1,32 @@
+  function register() {

Needs a "public".

Otherwise this is looking a lot better now.

effulgentsia’s picture

FileSize
1.16 KB
7.33 KB

Ok. This adds the public, and removes the @todo.

The question is whether this code would cause problems for a 204 or similar.

204 is an explicit statement to the user agent that there is no content, so I think the patch as-is is correct to not decorate that with blocks. 304 is perhaps more interesting, but we'll deal with that when we implement HTTP-compliant block-level caching.

Status: Needs review » Needs work

The last submitted patch, user-route-13.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

#13: user-route-13.patch queued for re-testing.

disasm’s picture

+++ b/core/lib/Drupal/Core/HtmlPageController.php
@@ -52,7 +52,7 @@ public function content(Request $request, $_content) {
+    $attributes = clone($request->attributes);

This is nitpicky, but technically clone is a PHP keyword, not a function, so it shouldn't have the () surrounding it.

All in all, this looks good. It's hacky, but I like the Access Denied shim for user_register_access on the register method. I never thought of doing that to start converting routes before #1793520: Add access control mechanism for new router system gets committed.

effulgentsia’s picture

FileSize
727 bytes
7.33 KB

Good catch. Thanks.

effulgentsia’s picture

Component: user.module » routing system
Issue tags: +WSCCI

Because this is one of the first routes converted, and because the patch fixes some routing system bugs in the process, moving to the routing system component and adding the WSCCI tag.

Lars Toomre’s picture

Attached are some notes from reading through the patch in #17.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.phpundefined
@@ -41,6 +42,10 @@ public function __construct(LanguageManager $language_manager) {
+    if ($event->getRequestType() !== HttpKernelInterface::MASTER_REQUEST) {
+      return;

Should not the @return directive be modified to indicate that NULL now can be a possible return?

+++ b/core/modules/user/lib/Drupal/user/UserRouteController.phpundefined
@@ -0,0 +1,32 @@
+ * @file
+ * Definition of Drupal\user\UserRouteController.

Should be 'Contains' instead of 'Definition of'.

+++ b/core/modules/user/lib/Drupal/user/UserRouteController.phpundefined
@@ -0,0 +1,32 @@
+  /**
+   * Returns the user registration form.

Missing a @return directive.

Crell’s picture

I'm good with this patch.

Attached path fixes the second two notes from Lars. For the first, event listeners don't return anything. There are no other return statements in this method so it will always return NULL anyway, which is something we don't currently document.

If the bot likes this, then we're RTBC here.

Crell’s picture

Oh, and I opened this follow-up, too: #1845402: Update menu link access checks for new router

disasm’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

So the idea is commit this one separately then the access control patch will add access for it?

sun’s picture

+++ b/core/lib/Drupal/Core/HtmlPageController.php
@@ -62,8 +62,12 @@ public function content(Request $request, $_content) {
+    // For successful (HTTP status 200) responses, decorate with blocks.
+    if ($response->isOk()) {
+      $page_content = $response->getContent();
+      $response = new Response(drupal_render_page($page_content));
+    }
...
+    return $response;

Hm. Do we really want to hard-code this decision this deep into the system? How would I override that decision if I'd want to have a fully rendered page including blocks on a non-200 response?

Right now, I think this is possible to achieve from within a module. I guess your answer will be "extend and override the class", but that can only be done once / by a single module, so if multiple modules need to override hard-coded decisions in the HtmlPageController, we're essentially stuck?

catch’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Do we really want to hard-code this decision this deep into the system? How would I override that decision if I'd want to have a fully rendered page including blocks on a non-200 response?

This would also be a regression for #423992: Remove show_blocks page variable, I'm really confused why it's included here too.

Crell’s picture

Not doing that meant that we get the inception bug again. (See #2.) That's a recurring bug that mostly is due to mismatch between old-style page handling subcontrollers and new-style. I expect that it will *eventually* go away once we convert the system over.

A 404 or 403 page will come back already fully formatted, I believe. It doesn't need to get re-rendered, which is where the inceptioning comes from. A normal 200 response comes back as just a body, so it does need to get re-page-wrapped. Again, I'm still holding out hope that SCOTCH changes all of this anyway. :-)

This issue still has access control on /user/register, but it's not done via the routing system but inline. It gets converted back to the routing system in #1793520: Add access control mechanism for new router system

catch’s picture

OK so with the 404 or 403 it's not a case of not displaying blocks, it's a case of not rendering the entire page twice inside itself? I think that workaround is OK as a stop-gap until things are converted but it could use comments and also a specific issue to track removing the workaround.

Also not sure if this means we're rendering the main content area twice then?

Inline access control yes I mis-typed but understood.

Crell’s picture

Correct. There's a lot of careful tapdancing going on right now to ensure that we don't end up inceptioning ourselves given that right now we have two overlapping pipelines with different assumptions. This is just a refinement of that tapdancing. Presumably once we only have one pipeline to worry about we'll be able to stop tapdancing and go back to the cha cha. I don't think that we need an issue specific to this workaround, when there's more inception-prevention code elsewhere anyway. A comment to that effect is fine, though.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I think this is RTBC, with or without another code comment

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK I think this could really have used the code comment, but I'm more interested in real examples in the router access patch, so committed/pushed this one.

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