Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Update: As of October 8, the legacy routing BC layer has been removed, and core includes page controllers for all new routes. The next step is to complete the conversions in the new page controllers to remove dependencies on procedural code and implement best practices. See https://groups.drupal.org/node/315498 for details. And an example of one of the subtasks that was done the new way is #1987778-105: Convert node_show() and node_page_view() to a new style controller
See also: #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase
Remaining tasks
Outstanding child issues:
Comment | File | Size | Author |
---|---|---|---|
#24 | callbacks.txt | 29.45 KB | xjm |
#24 | form_builders.txt | 15.49 KB | xjm |
Comments
Comment #1
Crell CreditAttribution: Crell commentedTagging.
Comment #1.2
ayelet_Cr CreditAttribution: ayelet_Cr commentedAdding more issues.
Comment #2
catchVery definition of critical.
Comment #4
webchickCan someone explain how this is the same or different than #1848648: [meta] Convert menu system to new routing system (also critical)? If this one is purely just about page callback conversions, seems like it ought to be "major" rather than "critical." While I don't think we can ship Drupal 8 with both the router parts of hook_menu() and module.routing.yml files (which I believe is what the other issue is saying), I can see us shipping with a few legacy page callbacks in .module files if we don't get them all done by release. It's very rare for modules to call other modules' page callback functions, so this OO conversion might even be doable in subsequent point releases, not sure.
Comment #5
mitchellshelton CreditAttribution: mitchellshelton commentedWSCCI Conversion Guide
Comment #6
xjmOops. :)
Comment #7
xjm@webchick, #1848648: [meta] Convert menu system to new routing system is about the menu system itself, whereas this issue is just about simple callbacks and form builders.
Comment #8
hawkeye.twolfToolbar and Translation modules have been checked (one issue each, added to the list).
EDIT:
Additional detail:
Comment #12
hawkeye.twolfAdd the following views issues to meta:
views: tests/views_test_data/views_test_data.module
views: views.module
views_ui: views_ui/views_ui.module
Comment #14
catchThe other issue is about the part-by-part re-implementation of old menu system features to the new one.
I'm not sure about shipping with some menu callbacks as functions, but it's not technically possible to do that at the moment. The bc-layer in the new router relies entirely on passing everything through to the old system which has to be removed, so it'd have to be added in there before that's even an option. Additionally all those items would still need to move into routing.yml anyway which could be tracked here.
Comment #15
xjmThanks @kgoel, @mitchellshelton, @vijaycs85, @derek.deraps, @cconrad, @shanethehat, and @sidharthap for your help checking for callbacks during core mentoring today!
I'm currently in the process of checking over the new issues that have been filed.
Comment #16
xjmtag monster--
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedI think less than 15% of Drupal core's page callback functions are in .module files. The vast majority are in .inc files. If we don't convert all of the ones in .inc files, then in addition to making sure that functions work as controller values in the new routing system, we'll need to also add a 'file' option to the routing.yml files to support loading those .inc files. Ugly as that may be, it may be preferable to delaying a D8 release if all other criticals get solved before this one. But I still think it makes sense to track this as critical in the meantime, with the understanding that the scope of the issue can change as we get closer to release.
For anyone who cares about us not resorting to some ugly and random mix of PSR-0 controllers with old-style include files, please help us get this issue 100% resolved :)
Comment #18
catchWe could also move the functions back to .module (although again I don't think this is a good idea in general).
Comment #19
webchickRight, just trying to work out a reasonable "plan B." Thanks to everyone for working on these patches. Keep 'em coming! :D
Comment #20
xjmSome of the issues added today are actually AJAX callbacks, and will probably need to be postponed on #1944472: Add generic content handler for returning dialogs. I'll update those issues.
Comment #21
andypostThere's a bunch of issues like #1872870: Implement a RoleListController and RoleFormController - what is a suggested way to convert them?
it seems no need to implement 2 issues - one to implement controllers and second to make another controller with routes
For config entities it seems better to make Generic implementation a-la #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) and implement special controller to inject config storage to it.
Any feedback welcome.
Comment #22
tim.plunkett@andypost, I think that is handled by #1909418: Allow Entity Controllers to have their dependencies injected
Comment #23
Crell CreditAttribution: Crell commentedI checked last week and verified that it would be possible to change the BC listener to allow for function controllers if needed. However, as others have noted I'd much rather not do so for the time being and try to get everything converted instead. Moving remaining page callbacks to the .module file and leaving them as-is would be plan Z IMO. It's technically possible, I just think it would be a really crappy end result.
Also, zOMG thanks to everyone who's been working on this and the linked issues while I was on vacation. :-)
Comment #24
xjmSo last week I believe we added issues for:
The two attached files contain all the non-form page callbacks in core, and then a rough grep that should get most or all of the form builders. We don't have anywhere near all of these filed. Let's make a table listing all of them so that we can keep track of issues that have different titles or target multiple callbacks, like the forum ones. I'll add a stub table to the summary.
Comment #25
xjmI added two stub tables. They won't include rows for callbacks that were removed before this morning's git pull. If you want you can add rows for fixed issues, but at least leave the current rows there, even if they are redundant. Fill in an issue in each row that removes the callback.
Comment #25.10
cconrad CreditAttribution: cconrad commentedRemoved all issues from the list below the tables which existed in one of the tables.
Also removed modules from the list which were marked as having no "hook_menu" or callbacks otherwise.
Comment #27
xjm@larowlan re: AJAX callbacks:
Comment #28
xjmI removed the
confirm_form()
conversions from the old list at the bottom of the summary if they were already listed on #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase.Comment #29
Crell CreditAttribution: Crell commentedOn the WSCCI meeting this morning we decided that we need to break this list up. Specifically, we need to separate module page callbacks from test module page callbacks; effort needs to be focused on the former, as the latter is easier to do post-1 July. Can someone help with that?
Comment #30
tim.plunkettI did the split after the meeting already. We now have 4 tables. (form/nonform, test/nontest)
Comment #31
valthebaldIs there documentation of advantages of controller based routing vs traditional, hook_menu() based?
I think that if there is justification for tremendous effort like this, it
a) should be documented somewhere, and this issue should contain link to this "somewhere"
b) hook_menu() should be claimed deprecated
Comment #32
xjmThanks @valthebald. This is a critical part of the web services initiative. Check out http://www.garfieldtech.com/blog/refocusing-wscci for an overview. :)
Also, these conversions do not entirely replace
hook_menu()
implementations, just the page callbacks. Note that the converted page callbacks still havehook_menu()
entries for their paths. So we're not at a point wherehook_menu()
should be deprecated.Edit: Also see http://drupal.org/node/1800686.
Maybe we could add these resources to the issue summary. :)
Comment #33
valthebald@xjm: thanks for clarification!
Comment #34
oenie CreditAttribution: oenie commentedI'm trying my best to help out on the controller business, but i'm running into confusing problems.
The fact is that the controllers that we have now are done in a wide variety of ways.
Their location: Controllers being dumped into the lib\Drupal\ directory , others into another subdir Controller
Implementation:
I'm being redirected to the WSCCI Conversion Guide, but it feels like this is incomplete.
I'm finding myself having to search for possible solutions to specific problems, but it's not always clear what/where to look for.
On top of that, issues are introduced that have impact on code that i find in my searches and use as a reference
(I submit a patch which uses the module_handler service, and then come across the issue
Replace calls to module_handler service with Drupal::moduleHandler())
I would really like to help in trying to get the WSCCI Conversion guide on par as a clear help and with reference to good, useful examples.
The book example is a start but it doesn't touch less trivial cases.
It would also be possible to replace a specific best practice, when something (like the replace issue above) changes.
This way, there would be a single point of information.
Am i too utopian here or is there anyone who can shed some light on best practices ?
I'm thinking specifically of available services to use, where to put files, ...
Comment #35
Crell CreditAttribution: Crell commentedHi Oenie.
Yes, the Conversion Guide is not yet complete. Mostly it's missing form handling and placeholders. Sorry about that. I want to get placeholders in there soon; forms are probably better written by Tim.
Strictly speaking, Controller classes can be named anything. By convention we've been trying to always put them in the Drupal\$module\Controller directory/namespace. If you see issues where that's not happening, please mark them needs work and/or fix them. :-)
The issue you reference above is a bit misleading. It's specifically about replacing drupal_container()->get('module_handler') with Drupal::moduleHandler(). Injecting the module handler service via create/__construct is still preferred over both, so please continue to do that.
You can also stop by #drupal-wscci in IRC most times and someone (usually Tim Plunkett, Daniel Werner, or myself) can try to help you out. (This week will be a bit crazy due to DrupalCon, of course.)
Comment #36
oenie CreditAttribution: oenie commentedHi Crell,
Daniel has been of real assistance to me yesterday.
As for the docs, i know you are all soooooo busy, just wanted to show it from my personal perspective.
I tried chipping in last year at Drupal Munich, but that turned out a bit too difficult at the time.
Now i feel like being able to do some helpful work.
Thanks for the clarification on the module_handler & Controller, it sounds like something that we might want to update in the docs ?
Especially the Controller location issue, since with all these ongoing conversions, the less extra work they create in the future the better.
Comment #37
berliner CreditAttribution: berliner commented@Crell: With reference to #1988802: [META] Rewrite test modules in system to provide better unit testing. and the note at the top of the page: wouldn't it be less confusing to replace the section "Test module form builders" with a link to the meta issue? I had just finished to convert
form_test_alter
and was about to post a patch, when I realized that there is a the mentioned meta issue and consequently no need for individual route conversions of test modules in the system module.Comment #38
Crell CreditAttribution: Crell commentedThat looks a bit different. This is specifically for moving from hook_menu to the new routing system. That issue seems like a broader attempt to make test modules unit testable... which seems rather redundant since test module only exist to support tests of other modules. :-)
Comment #39
xjmWe cannot wontfix issues like #1987832: Convert system_test callbacks to a new style controller. This issue is critical; we shouldn't block criticals on cleanups. Let's please reopen them so that work can continue? If someone wants to work on rewriting tests, fine, but that cleanup will take a lot longer than we can afford to wait to get rid of the old routing system.
Comment #40
xjmThe core maintainers have agreed that these conversions can and should continue to happen after July 1. Declaring routes via
hook_menu()
in the D7 fashion is deprecated and will almost certainly be removed before release.Comment #41
xjmtag monster--
Comment #42
tstoecklerOpened #2074155: Convert user/%user/edit to a route but couldn't find it in the issue summary yet.
Comment #43
xjmThanks everyone for awesome work on these conversions so far!
In order to expedite the removal of the old routing system, we're changing the conversion process on these issues a bit. See: A faster process for Drupal 8 routing conversions.
What this means for existing issues is that any conversion patch that is committable by September 14 can continue on as it has been. After that, we'll ask you to split your work into two patches -- one that adds the route definition and controller class/method, but does not touch the old page or form callbacks; and then a separate, postponed followup issue with any other work from your conversion to date. (Also, feel free to start using this process on issues assigned to you now if you prefer.)
Comment #44
webchickOk, https://drupal.org/project/issues/search/drupal?status[]=14&issue_tags=W... is now an empty queue, and it's also September 15 in about 7 minutes. So this officially kick off the "just convert routes as fast as possible" methodology.
Thinking about it, though, I wonder if we could do this as one humungous patch for all remaining routes (ideally, this could be scripted, if not, then hacked on in a sandbox among 2-3 people perhaps), because:
a) It is much less effort-intensive this way. ONE patch to create/review/commit, not 50+.
b) We wouldn't need to open another 50+ issues for the "step B" patches, and we could preserve all of the existing comments in those issues that are about taking the "boiling the ocean" approach and let them continue doing that once the routes are moved.
c) Frankly, we don't have another 6 months to figure this out in individual little issues. We need to get this done. If we get it done before Prague, so much the better, because we'll be able to go in there knowing the really hairy menu router problems we still need to figure out. I don't see any way this gets committed before Prague unless it's one patch.
Comment #45
webchickThat's what's left according to grep.
Comment #46
webchickPlease note that #1981368: Convert all routes to 'module_name.foo_bar' naming convention just got committed. I've updated the existing change notices, and the WSCCI conversion guide accordingly.
Comment #47
effulgentsia CreditAttribution: effulgentsia commentedIf I were one of the people trying to roll the patch for this (which I probably won't be, so feel free to ignore), I would split it into 4 patches:
- test module non-forms
- test module forms
- non-test module non-forms
- non-test module forms
And then, if any specific ones required a feature that does not yet exist in HEAD (e.g., a node_access() access checker compatible with the new routing system), then I would split those off into separate patches (possibly lumping all conversions that are blocked on the same missing feature into the same patch).
Comment #48
tim.plunkettI'm taking a crack at #2089635: Convert non-test non-form page callbacks to routes and controllers, the other 3 issues need to be opened.
Comment #49
disasm CreditAttribution: disasm commentedAnd here's non-test form page callbacks: #2089671: Convert non-test form callbacks to new form controller
Comment #50
webchickAlso note that #2051097: Change "pattern" to "path" in *.routing.yml files just got committed as well.
Comment #51
tim.plunkett#2091691: Convert test non-form page callbacks to routes and controllers is the 3rd of the 4 issues. Remaining is the form callbacks in test modules.
Comment #52
disasm CreditAttribution: disasm commented#2102653: Convert test form callbacks to new form controller is the final of the 4 issues.
Comment #53
webchick#2102653: Convert test form callbacks to new form controller was just committed! :)
We're getting there now. :)
I still see:
#1972990: Convert tracker_page() to a Controller
#1938884: Replace the fallback user listing with a list controller
#2029569: Convert node_admin_nodes to a new-style Controller
Looks like system.api.php needs some clean-up, and then not sure what's happening in design_test.module.
Comment #54
tim.plunkett#2027115: Allow views to override existing routing items replaces the user_admin_account one, and #2021161: Replace the fallback node listing with a list controller replaces the node_admin_nodes one which is blocked on the views one.
design_test module is 100% unused, see #2037569: Remove design_test module
Comment #55
webchickSomething that keeps coming up when I talk to people about D8 "in the field" is the current mishmash we have where some controllers are in classes with OO methods, and others are in classes with dumb wrappers around procedural code is creating a really confusing environment for learning the new routing system. :\ I think it'd be great to have one more "mega patch" that moves the body of those functions into the right places (and drop the old procedural functions) without also trying to boil the ocean of Dependency Injection blah blah blah.
Comment #56
dawehnerI am not sure whether it is worth to break all the conversion patches again.
Comment #57
vijaycs85Updating new sub-task - #2147009: Convert all forms in ajax_forms_test module to FormInterface
Comment #58
vijaycs85Cleaning up sub-task list with completed & remaining session
Comment #59
vijaycs85Updating more items into completed list.
Comment #61
webchickNote that there was a change notice for this at https://drupal.org/node/2119699 with a before/after table which was being populated by the functions within individual conversion issues such as #1984702: Convert menu.module's page callbacks to Controllers which seemed like sheer, unadulterated madness to me.
I adjusted it to instead show a sample conversion of a D7 page callback => D8 controller that I blatantly stole from http://effulgentsia.drupalgardens.com/content/drupal-8-hello-oop-hello-w.... Someone probably should check my work, though: https://drupal.org/node/2119699/revisions/view/6890847/6891155
Comment #68
xjmThanks everyone for your work on resolving these issues!
Now that we've removed the BC layer entirely in #2107533: Remove {menu_router}, the remaining work is to improve these various controllers to remove dependencies on procedural code, use DI where appropriate, etc. This remaining cleanup work doesn't block release, so downgrading to major. We also are probably not going to treat individual controllers as public API, so the changes in these issues can likely continue to go in after beta1, rc1, etc. (at maintainer discretion of course).
What we need most now for these issues is good architectural review for the patches. Many of them have been being rerolled for a long time (and probably need to be rerolled again for PSR-4). I'd suggest any interested reviewers start with the non-test, non-view issues.
Comment #69
jackbravo CreditAttribution: jackbravo commentedI was checking #2078867: Convert _form_test_* functions to classes which is linked here, but seems like I said on [comment 23](https://drupal.org/node/2078867#comment-8856823) seems like just changing the tests to the new code is not enough because they are not being run with drush test-run or displayed in the Testing module.
Comment #70
xjmRemoving the completed list from the summary, since it's a bit overwhelming.
Comment #72
xjmComment #73
tim.plunkettComment #74
tim.plunkettAdded one for #2302525: Convert file_module_test_form to a class
I'm tracking the forms in #2268761: Remove support for function-based forms.
Comment #75
Mile23Added
user_cancel_confirm()
.Comment #76
Mile23No, they don't, since they no longer exist.
Comment #77
aspilicious CreditAttribution: aspilicious commentedComment #78
aspilicious CreditAttribution: aspilicious commentedComment #79
aspilicious CreditAttribution: aspilicious commentedComment #80
cosmicdreams CreditAttribution: cosmicdreams commentedremoved #1979040: Rewrite XML RPC module to services and plugins because it's assigned to a contrib project now.
Comment #81
aspilicious CreditAttribution: aspilicious commentedComment #82
alexpottAll the child issues have been committed. Time for this one to be set to fixed!
Comment #83
Crell CreditAttribution: Crell commentedOMG WTF BBQ zOMG!
Amazing work everyone!
Comment #84
webchickWow! Fantastic! Great work, all!
Comment #85
jibranCan we revert back to old massive issue summary? Yay!!! for the fix.
Comment #86
kim.pepper/me weeps gently
Comment #87
vijaycs85yay!
Comment #88
jibranThis is the relevant revision https://www.drupal.org/node/1971384/revisions/7382361/view
Comment #89
vijaycs85#88: :) there were only 2 issues.
1. xmlrpc is out of core.
2. comment admin page to views isn't really part of this meta(i.e. converting callback to controller).
I think we are good with the status of this meta issue.