Currently, routes can't really cope with forms. You can write a controller or _content controller that calls drupal_get_form() itself, but there's really no equivalent of 'page callback' => 'drupal_get_form' yet. That's a pain, especially since so many of our routes in core are admin forms.
Well, no more!
This patch (coming as soon as I have a nid) adds a new type of wrapping _controller, similar to HtmlPageController, called HtmlFormController. It activates when you have a _form key on your route instead of a _content. It works pretty much the same way as the old page callback trick does; in fact, the body of the only method in HtmlFormController is only a minor modification from drupal_get_form().
There's two key caveats:
- It only works with the new FormInterface-based forms (see http://drupal.org/node/1932058), not with forms defined in a function. Given that this lets us move forms out of .module files and into lazy-loaded classes, I consider this a feature.
- Form arguments are handled with a separate _form_arguments key in the route, but I haven't tested that yet so I'm not sure if it works. :-)
Also, while right now forms are all instantiated straight, there's no reason we cannot let them have the same factory method as controllers as soon as #1915774-17: Decide whether core route controllers should generally/always be DIC services or not gets in. That would allow us to have nicely injected form builder objects, too. Win!
So far I've just converted one simple admin form, and haven't updated any tests. It may or may not break testbot, but it's after 2 am so I want to get this up for review (and collaborators, please!) and go sleep.
Tim Plunkett: I told you it was possible. :-)
Comment | File | Size | Author |
---|---|---|---|
#16 | forms-1934832-16.patch | 33.73 KB | tim.plunkett |
#16 | interdiff-whole.txt | 1.61 KB | tim.plunkett |
#16 | interdiff-partial.txt | 1.03 KB | tim.plunkett |
#14 | 1934832-form-controller.patch | 34.17 KB | Crell |
#12 | forms-1934832-12.patch | 33.22 KB | tim.plunkett |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch.
Comment #2
tim.plunkettWorking on this some more.
Comment #3
tim.plunkettStill needs actual explicit tests, but now supports the main three use cases and has implicit testing for:
Instead of a key _form_arguments, it uses reflection to determine what buildForm() needs.
That said, we might want to leave the system_site_maintenance_mode() conversion to #1925660: Convert system's system_config_form() to SystemConfigFormBase and just add explicit tests. Same goes for the Views UI stuff.
Comment #4
tim.plunkettOkay, here it is with tests now, for each of the three use cases needed right now in core.
I've also attached a patch without the changes to non-test forms, in case we want to reduce the scope and amount of conflicts. I'd be happy with either.
Comment #5
tim.plunkettWhoops, the no-conversions one still had the MaintenanceModeForm in it.
Comment #6
Crell CreditAttribution: Crell commentedThis is vestigial. We are using the container now. :-)
Otherwise, I'm happy here. I can go either way on including MaintenanceModeForm, whatever committers prefer. Let's fix the doc and get this in.
Comment #7
tim.plunkettHere is a final patch with the outdated @todo removed.
Comment #8
Crell CreditAttribution: Crell commentedWin!
Comment #9
catchNeeds updating after #1915774: Decide whether core route controllers should generally/always be DIC services or not.
Comment #10
tim.plunkettI'll need to add an explicit test for it.
Comment #11
tim.plunkettNow that the other issue was done, there was no sense in not converting the old-style getForm() controllers.
Comment #12
tim.plunkettI went back to the system_config_form() conversions to see how this patch worked with them, and it is pretty awesome for DX, with just one downside: a lot of boilerplate is needed the inject config.factory and avoid config() calls.
So, I've enhanced SystemConfigFormBase by giving it a default constructor and create() static method, and have it implement ControllerInterface.
From when I originally set up the system_config_form() meta, more than 75% of the forms don't have any dependencies besides config.factory, so this saves a lot of code, and still saves some extra work for those that do.
I proposed this to Crell and he thought it was reasonable.
I promise I'm done (unless tests fail for some reason).
Comment #14
Crell CreditAttribution: Crell commentedThe new ControllerInterface on forms only works for directly referenced forms with the new router, not arbitrary drupal_get_form() calls. That is, oops, forgot to convert that. :-)
If this comes back green, it's RTBC.
Comment #15
amateescu CreditAttribution: amateescu commented:P
Otherwise looks great to me.
Comment #16
tim.plunkettFixed some of Crell's missteps, also provide an interdiff for his changes as well.
Comment #17
Crell CreditAttribution: Crell commentedFor the love of Druplicon, I DIDN'T COMMIT THAT! I verified it, too! Ugh. (And the menu change is me getting ahead of myself. 'route' will be the new way of doing that as of #1845402: Update menu link access checks for new router. Boy I hope these don't conflict...)
Well, at least between the two of us at least one of us can RTBC this thing in the morning. :-)
Comment #18
Crell CreditAttribution: Crell commentedOK, I think we're done for reals now.
Comment #19
webchickThis seems reasonable to me, and:
is much nicer DX-wise than:
Looks like it'll conflict with #1845402: Update menu link access checks for new router, though, but that one's not ready. :\
Committed and pushed to 8.x, in preparation of Global Drupal Sprint Weekend. Thanks! We'll need to adjust the existing change notice for the router system to reflect this.
Comment #20
webchickComment #21
Crell CreditAttribution: Crell commentedI've added a change notice to http://drupal.org/node/1800686.
Comment #22
Crell CreditAttribution: Crell commentedComment #23
Barnettech CreditAttribution: Barnettech commentedHere's a first attempt at a change control page.
Summary:
Before
Prior to Drupal 8 in hook_menu you could use drupal_get_form and specify which form to display.
After
In Drupal8 with the new routing system you can now also display a form in much the same manner using HtmlFormController, rather than HtmlPageController would handles return html, and non-form content.
Comment #24
Barnettech CreditAttribution: Barnettech commentedPlease review the above attempt at a change control for this. thanks!
Comment #25
tim.plunkett@barnettech, there is already a change notice now, if you'd like to improve it, you can edit it directly.
Comment #26
Barnettech CreditAttribution: Barnettech commentedNope, sorry I missed Crell's note above, this issue was in http://core.drupalofficehours.org/ I'll close it out. Lol my post was very pale in comparison to Crell's who's more knowledgeable on the subject quite obviously. Well at least it's forcing me to get initiated with the new stuff in D8 and how to write change controls etc. Cheers and thanks for your patience as I try to get involved and helpful with Drupal core.