Posted by effulgentsia on November 16, 2012 at 10:12pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | routing system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | WSCCI |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| user-route.patch | 2.5 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] 48,166 pass(es), 17 fail(s), and 14 exception(s). | View details |
Comments
#1
The last submitted patch, user-route.patch, failed testing.
#2
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 :)
#3
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.
#4
The last submitted patch, user-route-3.patch, failed testing.
#5
+++ 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.
#6
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.
#7
It does. $request->getPathInfo() doesn't, and as I understand it, shouldn't.
#8
Right, getPathInfo() is the raw request URI, which is why we should rarely if ever be using that.
#9
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?
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.
#10
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.
#11
Ok, how's this?
#12
+++ 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.
#13
Ok. This adds the public, and removes the @todo.
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.
#14
The last submitted patch, user-route-13.patch, failed testing.
#15
#13: user-route-13.patch queued for re-testing.
#16
+++ 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.
#17
Good catch. Thanks.
#18
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.
#19
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.
#20
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.
#21
Oh, and I opened this follow-up, too: #1845402: Update menu link access checks for new router
#22
#23
So the idea is commit this one separately then the access control patch will add access for it?
#24
+++ 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?
#25
This would also be a regression for #423992: Remove show_blocks page variable, I'm really confused why it's included here too.
#26
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
#27
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.
#28
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.
#29
I think this is RTBC, with or without another code comment
#30
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.
#31
Automatically closed -- issue fixed for 2 weeks with no activity.