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.
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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 1843084-user-register-route.patch | 7.42 KB | Crell |
#17 | user-route-17.patch | 7.33 KB | effulgentsia |
#17 | interdiff.txt | 727 bytes | effulgentsia |
#13 | user-route-13.patch | 7.33 KB | effulgentsia |
#13 | interdiff.txt | 1.16 KB | effulgentsia |
Comments
Comment #2
tnightingale CreditAttribution: tnightingale commentedHeh 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 :)
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedThanks 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.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #6
Crell CreditAttribution: Crell commentedUgh, that nonsense again.
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.
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.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedIt does. $request->getPathInfo() doesn't, and as I understand it, shouldn't.
Comment #8
Crell CreditAttribution: Crell commentedRight, getPathInfo() is the raw request URI, which is why we should rarely if ever be using that.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedSo does that mean:
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.
Comment #10
Crell CreditAttribution: Crell commentedFor 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.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedOk, how's this?
Comment #12
Crell CreditAttribution: Crell commentedI 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.
Needs a "public".
Otherwise this is looking a lot better now.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedOk. 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.
Comment #15
effulgentsia CreditAttribution: effulgentsia commented#13: user-route-13.patch queued for re-testing.
Comment #16
disasm CreditAttribution: disasm commentedThis 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.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedGood catch. Thanks.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedBecause 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.
Comment #19
Lars Toomre CreditAttribution: Lars Toomre commentedAttached are some notes from reading through the patch in #17.
Should not the @return directive be modified to indicate that NULL now can be a possible return?
Should be 'Contains' instead of 'Definition of'.
Missing a @return directive.
Comment #20
Crell CreditAttribution: Crell commentedI'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.
Comment #21
Crell CreditAttribution: Crell commentedOh, and I opened this follow-up, too: #1845402: Update menu link access checks for new router
Comment #22
disasm CreditAttribution: disasm commentedComment #23
catchSo the idea is commit this one separately then the access control patch will add access for it?
Comment #24
sunHm. 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?
Comment #25
catchThis would also be a regression for #423992: Remove show_blocks page variable, I'm really confused why it's included here too.
Comment #26
Crell CreditAttribution: Crell commentedNot 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
Comment #27
catchOK 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.
Comment #28
Crell CreditAttribution: Crell commentedCorrect. 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.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commentedI think this is RTBC, with or without another code comment
Comment #30
catchOK 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.