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.
Change notice is at here
Comment | File | Size | Author |
---|---|---|---|
#80 | interdiff.txt | 2.79 KB | tim.plunkett |
#80 | system-1987814-80.patch | 26.73 KB | tim.plunkett |
#75 | interdiff.txt | 915 bytes | tim.plunkett |
#75 | system-1987814-75.patch | 26.95 KB | tim.plunkett |
#70 | system-1987814-70.patch | 26.06 KB | tim.plunkett |
Comments
Comment #1
lliss CreditAttribution: lliss commentedComment #2
lliss CreditAttribution: lliss commentedFirst go at it. Patch attached.
Comment #3
lliss CreditAttribution: lliss commentedComment #5
lliss CreditAttribution: lliss commentedOkay. Tried to get to clever and use a single route to handle a bunch of different paths. But these started clobbering the others further down the url path. admin/config/content would work but admin/config/content/something would not. So I'm just declaring a single route for each item now. This seems redundant.
Comment #7
kim.pepperlliss, are you still working on this?
Comment #8
lliss CreditAttribution: lliss commentedI'm willing to but I couldn't make sense of the last round of failures from the test bot. Any guidance would be appreciated.
Comment #9
tim.plunkettI'm not sure if they were added since the last patch, but system_admin_menu_block_page() is used as a page callback in both core/modules/user/user.module and core/modules/system/tests/modules/menu_test/menu_test.module.
Those would need to be converted.
Comment #10
juampynr CreditAttribution: juampynr commentedConverted remaining callbacks. Also removed function bookRender().
I am not sure about how to change the following since now we have several routes pointing to the same controller.
Comment #11
juampynr CreditAttribution: juampynr commentedChanging status.
Comment #13
longwaveMany of the comment blocks in #10 need reformatting so they wrap correctly at 80 characters.
Comment #14
juampynr CreditAttribution: juampynr commentedFixed comment lengths. I still need some help with #10.
Comment #15
dawehnerThis is really cool.
What about just returning a render array?
This is pretty much awesome! In controller for all content
I'm wondering how we can get rid of this additional coupling between user and system module.
Comment #16
juampynr CreditAttribution: juampynr commentedNow using a render array to return the contents.
Also moved the logic to SystemManager class.
Comment #18
kim.pepperOne minor nitpick:
Should be
Comment #19
juampynr CreditAttribution: juampynr commented#16: drupal-system-7391414-16.patch queued for re-testing.
Comment #21
juampynr CreditAttribution: juampynr commentedNice catch @kim.pepper! Fixed. I have run the tests and confirmed that they are broken because of the changes of the patch, so I am going to dive into them to see what is broken.
Comment #22
juampynr CreditAttribution: juampynr commentedOops, uploaded the wrong version of the patch. Here it is.
Comment #23
juampynr CreditAttribution: juampynr commentedWhoa! and I was using wrong issue numbers patch. I think I need to sleep :S
I figured out why there were so many failing tests: I needed to add the SystemManager to the SystemController with a use statement and make a method public in the former.
Looking forward to see how many remaining tests are to be fixed. I still have the issue I mentioned at #10.
Comment #25
juampynr CreditAttribution: juampynr commentedGreat, I am back to 5 failing assertions. I am going to figure out how #10 works.
Tips are more than welcome.
Also, I am assigning this ticket to myself.
Comment #26
juampynr CreditAttribution: juampynr commentedRe-rolling.
Comment #28
juampynr CreditAttribution: juampynr commentedFixed a permission thanks to @tim.plunkett.
Comment #30
juampynr CreditAttribution: juampynr commentedRe-rolling again.
Comment #32
juampynr CreditAttribution: juampynr commentedRerolling.
Comment #33
juampynr CreditAttribution: juampynr commentedControllerInterface has changed its namespace. Rerolling again.
Comment #35
dawehner__construct needs some docs. While you are here, it might be cooler to have __construct() before create()
Comment #36
juampynr CreditAttribution: juampynr commentedRe-rolled and made the suggested changes at #35. I also saw that one of the remaining two tests failing passed locally so I am giving it another try for the test bot.
Comment #38
vijaycs852 fails:
1. This might not be valid test case anymore. But still out for discussion.
2.
HTTP response expected 403, actual 404 Browser MenuTest.php 635 Drupal\menu\Tests\MenuTest->verifyAccess()
don't see anything in that line number of that file :(
Comment #39
juampynr CreditAttribution: juampynr commentedThe related menu item is not inheriting the controller. Has hook_menu() changed how inheritance works in Drupal 8?
Comment #40
dawehnerlet's find a better route name like user_admin_index or something similar. user_accounts really reminds you on user/{} or admin/people
Comment #41
juampynr CreditAttribution: juampynr commentedChanged user_accounts route to user_admin_index as requested at #40.
Comment #43
juampynr CreditAttribution: juampynr commentedFixed one of the failing tests.
Comment #45
juampynr CreditAttribution: juampynr commentedDiscussed it with @dawehner. We do not need to check for callback inheritance since Controllers do not support it. I am removing the testing hook_menu() entry and the related test.
Comment #47
juampynr CreditAttribution: juampynr commentedAdding again help module as a dependency of the failing test (that is why it fails).
Comment #48
kushrohra CreditAttribution: kushrohra commentedRe Rolled patch no 47 and create a new patch to solve above problem
Comment #50
juampynr CreditAttribution: juampynr commented#48: drupal-system-1987814-48.patch queued for re-testing.
Comment #52
disasm CreditAttribution: disasm commentedThis review applies to #47 which doesn't need rerolled, it applies cleanly. The patch in #48 looks like it might have lost some code from #47 because the file size is smaller. Also, has an ambiguous statement about solving a problem, but no interdiff showing what was done, so my opinion is future work should be done based off of #47.
Not going to select the entire file, but from previous conversions I've worked on, putting a blank line between each route makes it much more legible. This whole file should have this done to it.
Other than that one nitpick, I don't see anything wrong with this patch and it passes all tests. I think it's ready for RTBC!
Comment #53
disasm CreditAttribution: disasm commentedThis review applies to #47 which doesn't need rerolled, it applies cleanly. The patch in #48 looks like it might have lost some code from #47 because the file size is smaller. Also, has an ambiguous statement about solving a problem, but no interdiff showing what was done, so my opinion is future work should be done based off of #47.
Not going to select the entire file, but from previous conversions I've worked on, putting a blank line between each route makes it much more legible. This whole file should have this done to it.
Other than that one nitpick, I don't see anything wrong with this patch and it passes all tests. I think it's ready for RTBC!
Comment #54
juampynr CreditAttribution: juampynr commentedAdded blank lines between each new route at system.routing.yml.
Comment #56
juampynr CreditAttribution: juampynr commented#54: drupal-system-1987814-54.patch queued for re-testing.
Comment #58
disasm CreditAttribution: disasm commentedNote that this patch fails on the test introduced in #1890878: Add modular authentication system, including Http Basic; deprecate global $user.
Comment #59
juampynr CreditAttribution: juampynr commentedWeird, I tried that test locally and it passed, hence I hit re-test.
I will have a look again.
Comment #60
tim.plunkettThe fail is legitimate and reproducible. I'm not sure why.
Rerolled, with some additions.
Comment #62
tim.plunkettWhoops.
Comment #63
jibranDrupal::request()->attributes->get('_account')->hasPermission('administer site configuration');
Comment #65
tim.plunkett#63 is best left for #1987810: Convert system_admin_config_page() to a new style controller.
Rerolled.
Comment #66
dawehnerAre we sure that menu_get_item will still work once the menu router is removed? We want to have the current request, so \Drupal::request() is the way to do it?
Missing docs ...
We also need to support the new router ...
user_access should be replaced \Drupal::currentUser()->hasPermisssion, but yeah this is kind of out of scope ...
Comment #67
tim.plunkettNo idea about the new router stuff, but this is 100% moved code.
1, 3, and 4 are out of scope.
Fixed the docs (2).
Comment #68
dawehnerWell, at least we know now that menu_get_item() should simple not be used at all.
Comment #70
tim.plunkettThe authentication providers were only properly restricted on legacy paths, not routes. This fixes it.
In addition, I'm removing an obscure hook_menu test that relied on this entire set of paths being page callbacks. It's time to cut the cord.
Comment #72
jibranSome minor issues.
@todo with the issue id will be nice here.
hmmm user_access can be replaced.
I think we can't call theme function directly now.
Comment #73
tim.plunkettThere is no issue for 72.1, until menu_get_item() itself is fixed. It's not specific to this usage at all.
72.2 and 72.3 are in #1987810: Convert system_admin_config_page() to a new style controller.
Comment #74
tim.plunkettThis issue: http://themetapicture.com/media/funny-gif-fix-light-bulb-car.gif
Comment #75
tim.plunkettIt was a wrong assumption in AuthTest. I ran this change by klausi and linclark.
Comment #76
jibranI think we should wait for #1987810: Convert system_admin_config_page() to a new style controller.
Comment #77
tim.plunkettYep.
Comment #78
jibranAnd it is in.
Comment #79
tim.plunkettWorking on it now.
Comment #80
tim.plunkettOkay, there was one remaining conversion, and the actual function itself. Not too bad!
Comment #81
jibranI don't know much about this change but i think we can replace it with current_user service. Not sure. Other then this it is RTBC for me.
Comment #82
tim.plunkett@jibran That is inside the subsystem that creates the current_user. Basically, current_user is broken, and I'm fixing it.
If we call it there, we'd get an infinite loop.
Comment #83
jibranOk then. Let's get this in.
Comment #84
tim.plunkett#80: system-1987814-80.patch queued for re-testing.
Comment #85
webchickOther than the hunk jibran pointed out in #81, answered in #82, this looks good to me. It's also a bit weird we're doing t() instead of $this->t() but this currently doesn't extend from any base classes, so that's fine.
Committed and pushed to 8.x. Thanks!
Comment #86
tim.plunkettComment #87.0
(not verified) CreditAttribution: commentedUpdated issue summary.