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.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#76 | layout-1978910-76.patch | 6.71 KB | tim.plunkett |
#76 | interdiff.txt | 2.15 KB | tim.plunkett |
#73 | layout-1978910-73.patch | 7.13 KB | kgoel |
#73 | interdiff.txt | 2.03 KB | kgoel |
#60 | layout-1978910-60.patch | 6.15 KB | tim.plunkett |
Comments
Comment #1
kgoel CreditAttribution: kgoel commentedgoing to work on this issue.
Comment #2
kgoel CreditAttribution: kgoel commentedPatch attached.
Comment #4
kgoel CreditAttribution: kgoel commentedTrying again.
Comment #6
kgoel CreditAttribution: kgoel commentedCreated new patch since drupal.org/node/1978908 was committed.
Comment #8
kgoel CreditAttribution: kgoel commentedTrying again.
Comment #9
kim.pepperLooking good, however, the access check callback is not being called anymore.
Need to create an implementation of AccessCheckInterface.
Comment #10
kgoel CreditAttribution: kgoel commentedThanks Kim!
I have added AccessCheckInterface function.
Comment #11
dawehner{node} probably does not work, and it also feels wrong at that point.
Needs docs
let's inject the layout manager into the layout controller...
Nitpick: empty line missing.
Comment #12
kgoel CreditAttribution: kgoel commentedAddressed all the issues under comment# 11.
Comment #13
kgoel CreditAttribution: kgoel commentedFixed some white spaces in layout controller.
Comment #14
dawehnerHere are just some nitpicks.
Let's remove the "Page callback"
Here is some trailing whitespace.
Comment #15
kgoel CreditAttribution: kgoel commentedRemoved page callback and white spaces.
Comment #17
dawehnerUps, this method should still have a documentation. sorry
Comment #18
kgoel CreditAttribution: kgoel commentedAdded doc block.
Comment #20
dawehnerYou forgot to add the LayoutAccessCheck class
Comment #21
kgoel CreditAttribution: kgoel commentedI have added LayoutAccessCheck in layout.services.yml. Not sure where else I need to add that class.
Comment #22
dawehnerYou need to write also the LayoutAccessCheck class, dealing with that.
Comment #23
kgoel CreditAttribution: kgoel commentedTrying again with LayoutAccessCheck.php
Comment #25
kgoel CreditAttribution: kgoel commented#23: 1978910-controller-23.patch queued for re-testing.
Comment #27
kgoel CreditAttribution: kgoel commented#23: 1978910-controller-23.patch queued for re-testing.
Comment #29
dawehner#23: 1978910-controller-23.patch queued for re-testing.
Comment #30
alexpottThis patch will fix #1986140: User 1 profile Access Denied (view and edit) -> Warning Illegal offset type in isset or empty yay!
Comment #31
xmacinfoMoving to major since some essential pages are blocked, like user profile.
Is there an issue to convert the same type of issue for the tracker module?
Comment #32
dawehnerLet's also inject the layout manager into the access class.
Comment #33
vijaycs85@dawehner, not sure how to inject it on access class (tried the same way we have in controller, but no luck). Fixed few request related issues and looks the page is working fine now.
Comment #34
Crell CreditAttribution: Crell commentedAdd an arguments block here to specify the service to inject into the access checker's constructor. See:
http://drupal.org/node/1953354
For an example. (You'd use a different service name, of course, but same idea.)
This change is wrong. Object properties should always be lowerCamelCase.
Comment #35
kgoel CreditAttribution: kgoel commentedAdded argument in the services.yml
Comment #36
kgoel CreditAttribution: kgoel commentedfixed white space
Comment #37
dawehnerPlease remove the extra whitespaces (http://vim.wikia.com/wiki/Highlight_unwanted_spaces would have helped you here :) ) If you copy this line to the access_check.layout your have the layout manager injected.
Comment #38
kgoel CreditAttribution: kgoel commentedComment #39
dawehnerLet me help you.
Comment #41
kgoel CreditAttribution: kgoel commented#39: drupal-1978910-39.patch queued for re-testing.
Comment #43
kgoel CreditAttribution: kgoel commentedComment #45
kim.pepper#43: 1978910-controller-43.patch queued for re-testing.
Comment #46
dawehnerThat's RTBC once the testbot gives an OK.
Comment #47
clemens.tolboomThis patch somehow fixes 'access denied' on user/1/view + user/1/edit .. how come?
Comment #48
xmacinfo@clemens.tolboom: Please look at #30.
Comment #49
clemens.tolboom@xmacinfo: tnx (and doh :-)
I can access admin/structure/templates after applying the patch. It needs Cache Clear twice but that's another issue I guess.
Comment #50
tim.plunkettWe prefix these with an underscore to prevent clashes with {placeholders}
Comment #51
kgoel CreditAttribution: kgoel commentedComment #52
dawehnerGood point tim!
Comment #53
tim.plunkettThis is no more major than other conversions
Comment #54
xmacinfoOther conversions are working before the conversion. Whereas here, Drupal is broken without the fix, which fix happens to be a conversion.
Comment #55
tim.plunkettIf this is a bug, it needs tests. I strongly disagree with the priority.
Comment #56
kgoel CreditAttribution: kgoel commentedxmacinfo - I am not sure if this patch is breaking something or this patch is fixing something which is already broken. Can you please elaborate little more?
Comment #57
xmacinfo@kgoel: “this patch is fixing something which is already broken”. See Comment #30.
In short, when we enable Layout, we get an access denied on any profile pages, even if we are user 1.
Comment #58
tim.plunkettThanks for describing the bug. Here's a regression test.
Comment #59
dawehnerThis variable should be documented.
access checkers should return static::ALLOW and static::DENY
Can you explain how this tests the patch?
Comment #60
tim.plunkettI didn't spend the time to figure out WHY the layout module returns 403 for all user pages, including user 1, but it is what happens, and the patch fixes the cause.
That's why I named it "testUserAccessRegression".
It probably needs more research and a real test.
Fixed the first two points.
Comment #61
dawehnerThe problem before has been the following: UserAccessController executes the hook_user_access, but layout_user_access was named like that.
Comment #62
Dries CreditAttribution: Dries commentedGiven that we're pretty much at the end of the development cycle and layout module isn't actually used it core, it may be better to remove layout module from core? Thoughts?
Comment #63
Crell CreditAttribution: Crell commentedThat's a SCOTCH question. I don't know if that would make life easier or harder for them. I defer to EclipseGc and Sam. Pinging them.
Comment #64
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #65
Dries CreditAttribution: Dries commentedWaiting to hear from EclipseGc before moving forward.
Comment #66
EclipseGc CreditAttribution: EclipseGc commentedFrom a code perspective, this seems fine, so I'm ++ there.
With regard to whether we should remove this, I would encourage us to tag it and come back before release. Nothing in core is using it yet, that's true, but Sam and I have made great strides towards using it, and looking at Dries' blog post, it seems we still could put in code that will use it so long as we're not changing APIs. I agree this should absolutely be removed before 8.0 (or sooner) if we don't have code that is using it, but it's also rather small, and self contained, so if we need to remove it later, it should be of minimal impact, and its presence allows sam and I to continue making progress without maintaining yet more code differences in princess.
If this is contentious, we can talk it out further, but the code itself here seems pretty straight forward, so I'd say we commit it.
Eclipse
Comment #67
xmacinfo@EclipseGc: Which tag should we use or create?
Comment #68
EclipseGc CreditAttribution: EclipseGc commentedDo we have a "review before release" or something?
Eclipse
Comment #69
alexpottLooks like we should be removing layout_page_view() from
layout.admin.inc
in this patch...Given that we have to reinstall drupal just to run this test and give that you are removing the offending hook... I think that this test is unnecessary. Funny bug though :)
Comment #70
star-szrWe have a revisit before release tag.
Comment #71
alexpottIn fact we should be removing the whole of layout.admin.inc
Comment #72
alexpottx-post
Comment #73
kgoel CreditAttribution: kgoel commentedComment #74
dawehnerI would vote to go with _permission: administer layouts and the custom layout access checker here, so there is just a single one.
On the route definition you have to set a new option: _access_mode: 'ALL' in order to work as you expect it to work.
Comment #75
dawehnerComment #76
tim.plunkettComment #77
jibranCan we use https://drupal.org/node/2019739 here.
Comment #78
tim.plunkettNo. If we wanted to use something, we'd use #type container, but it doesn't expect a render array here.
Comment #79
safetypinI just arrived here from #1986140. I (just this morning) downloaded d8, created a new database, performed a minimal installation, enabled the layout module and ran into the access denied error mentioned in that thread. I just downloaded this patch file, applied it, and seems to have resolved that problem for me.
I'm not sure what else needs to be tested..
Comment #80
xmacinfo@Dries and @alexpott are both suggesting to remove the layout module from core, whereas @EclipseGC would like the patch to be commited and decide near release if we keep layout.
Adding the “revisit before release” tag as requested.
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedOpened #2053879: Remove layout.module from core, so removing the "revisit before release" tag from this issue.
Comment #82
ergophobe CreditAttribution: ergophobe commentedJust wanted to say that I updated from git on 11 Aug 2013 and applied the patch in #76 and it fixes all issues I was having #2062811: Access Denied to User 1 to View User Pages, Operations Column empty
- access denied on user pages
- consequent missing buttons on People page
- illegal offset warnings #1986140: User 1 profile Access Denied (view and edit) -> Warning Illegal offset type in isset or empty
Comment #83
Crell CreditAttribution: Crell commentedIt sounds from #79 like this patch still has issues, although it looks good to me visually.
Whether layout stays in core or moves to contrib, this code will need to get written anyway so we may as well do it here while the module still lives here.
Comment #84
ergophobe CreditAttribution: ergophobe commentedCrell - I think you mean the patch in #76.
#79 is a test reporting the issue encountered on a minimal install and fixed by the patch in #76.
#82 is reporting that the patch fixed all related issues for me based on a 8.x git pull of 11 Aug.
That's two positive tests, plus I assume tim.plunket wouldn't have posted the patch if it wasn't working for him. So with three people reporting the issue fixed and nobody contrary, it seems like it would be worth committing and closing this issue.
It seems like the issues are "philosophical" rather than practical (in other words, while deciding whether this should be in core or not, committing it will make it simpler for people to test other issues and patches - it probably took me at least an hour of looking at error messages, digging into code, posting issues, digging further and finding this patch here and then my time was up and I don't even remember what issue I was working on at the time).
Comment #85
Crell CreditAttribution: Crell commentedAh, OK. I misunderstood the context of #79. Let's commit it then.
Comment #86
catchIf we're going to remove layout, I'd rather just remove it asap than be working on issues in the core queue when there's about 90 RTBC patches per week. Moving over to Dries.
Comment #87
xmacinfoAgreed! And this affects #2053879: Remove layout.module from core.
Comment #88
Crell CreditAttribution: Crell commentedFine, but this is one of those RTBCs. Can we get this committed before more work needs to be done on it, wherever it would be done?
Comment #89
webchick#76: layout-1978910-76.patch queued for re-testing.
Comment #90
webchickSince this'll need to happen regardless if Layout module gets removed from core, I figured it best to just go ahead and commit it.
Committed and pushed to 8.x. Thanks!
Comment #91
webchick