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. :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Active » Needs review
FileSize
9.51 KB

And patch.

tim.plunkett’s picture

Assigned: Crell » tim.plunkett
Issue tags: +Needs tests, +VDC

Working on this some more.

tim.plunkett’s picture

FileSize
12.51 KB
16.03 KB

Still 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.

tim.plunkett’s picture

Okay, 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.

tim.plunkett’s picture

FileSize
13.31 KB

Whoops, the no-conversions one still had the MaintenanceModeForm in it.

Crell’s picture

+++ b/core/lib/Drupal/Core/HtmlFormController.php
@@ -0,0 +1,81 @@
+ * This doesn't use the container yet, but see the @todo inline below for where
+ * we'll be using it shortly.

This 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.

tim.plunkett’s picture

Title: Allow forms in routes » Provide a dedicated approach for using forms in routes
FileSize
13.2 KB
512 bytes

Here is a final patch with the outdated @todo removed.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Win!

catch’s picture

Status: Reviewed & tested by the community » Needs work
tim.plunkett’s picture

Assigned: Crell » tim.plunkett

I'll need to add an explicit test for it.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
10.38 KB
21.67 KB

Now that the other issue was done, there was no sense in not converting the old-style getForm() controllers.

tim.plunkett’s picture

FileSize
11.99 KB
33.22 KB

I 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).

Status: Needs review » Needs work

The last submitted patch, forms-1934832-12.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
34.17 KB

The 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.

amateescu’s picture

+++ b/core/modules/system/tests/modules/form_test/form_test.info.yml
@@ -3,4 +3,4 @@ description: 'Support module for Form API tests.'
-hidden: true
+#hidden: true

:P

Otherwise looks great to me.

tim.plunkett’s picture

Fixed some of Crell's missteps, also provide an interdiff for his changes as well.

Crell’s picture

For 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. :-)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

OK, I think we're done for reals now.

webchick’s picture

Title: Provide a dedicated approach for using forms in routes » Change notice: Provide a dedicated approach for using forms in routes
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

This seems reasonable to me, and:

+++ b/core/modules/views/views_ui/views_ui.routing.ymlundefined
@@ -15,14 +15,14 @@ views_ui.add:
-    _controller: '\Drupal\views_ui\Form\BasicSettingsForm::getForm'
+    _form: '\Drupal\views_ui\Form\BasicSettingsForm'

is much nicer DX-wise than:

+++ b/core/modules/system/tests/modules/form_test/form_test.moduleundefined
@@ -28,16 +28,9 @@ function form_test_menu() {
-    'page callback' => 'form_test_system_config_form',

@@ -377,20 +370,6 @@ function form_test_permission() {
-/**
- * Page callback: Displays a form built from SystemConfigForm.
- */
-function form_test_system_config_form() {
-  return drupal_get_form(new SystemConfigFormTestForm());

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.

webchick’s picture

Issue tags: +SprintWeekend2013
Crell’s picture

Title: Change notice: Provide a dedicated approach for using forms in routes » Provide a dedicated approach for using forms in routes
Priority: Critical » Normal
Issue tags: -Needs change record

I've added a change notice to http://drupal.org/node/1800686.

Crell’s picture

Status: Active » Fixed
Barnettech’s picture

Issue tags: -WSCCI, -VDC, -FormInterface

Here's a first attempt at a change control page.

Summary:

  • In Drupal 8 using the new router system you will be able to call HtmlFormController which will be much like the functionality of drupal_get_form in Drupal 7 and prior versions

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.

Barnettech’s picture

Status: Fixed » Needs review

Please review the above attempt at a change control for this. thanks!

tim.plunkett’s picture

Status: Needs review » Fixed
Issue tags: +WSCCI, +VDC, +FormInterface

@barnettech, there is already a change notice now, if you'd like to improve it, you can edit it directly.

Barnettech’s picture

Nope, 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.

Automatically closed -- issue fixed for 2 weeks with no activity.