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

Files: 
CommentFileSizeAuthor
#16 forms-1934832-16.patch33.73 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 52,459 pass(es).
[ View ]
#16 interdiff-whole.txt1.61 KBtim.plunkett
#16 interdiff-partial.txt1.03 KBtim.plunkett
#14 1934832-form-controller.patch34.17 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 52,487 pass(es).
[ View ]
#12 forms-1934832-12.patch33.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 52,492 pass(es), 1 fail(s), and 6 exception(s).
[ View ]
#12 interdiff.txt11.99 KBtim.plunkett
#11 forms-1934832-11.patch21.67 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 52,516 pass(es).
[ View ]
#11 interdiff.txt10.38 KBtim.plunkett
#7 interdiff.txt512 bytestim.plunkett
#7 forms-1934832-6.patch13.2 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 52,445 pass(es).
[ View ]
#5 forms-1934832-5.patch13.31 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 52,594 pass(es).
[ View ]
#4 forms-1934832-4.patch25.41 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 52,589 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 interdiff.txt9.38 KBtim.plunkett
#4 forms-1934832-4-no-conversions.patch15.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 forms-1934832-3.patch16.03 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 52,648 pass(es).
[ View ]
#3 interdiff.txt12.51 KBtim.plunkett
#1 1934832-form-controller.patch9.51 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 52,562 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new9.51 KB
PASSED: [[SimpleTest]]: [MySQL] 52,562 pass(es).
[ View ]

And patch.

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

Working on this some more.

StatusFileSize
new12.51 KB
new16.03 KB
PASSED: [[SimpleTest]]: [MySQL] 52,648 pass(es).
[ View ]

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.

Assigned:tim.plunkett» Crell
Issue tags:-Needs tests+FormInterface
StatusFileSize
new15.54 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new9.38 KB
new25.41 KB
FAILED: [[SimpleTest]]: [MySQL] 52,589 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new13.31 KB
PASSED: [[SimpleTest]]: [MySQL] 52,594 pass(es).
[ View ]

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

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

Title:Allow forms in routesProvide a dedicated approach for using forms in routes
StatusFileSize
new13.2 KB
PASSED: [[SimpleTest]]: [MySQL] 52,445 pass(es).
[ View ]
new512 bytes

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

Status:Needs review» Reviewed & tested by the community

Win!

Status:Reviewed & tested by the community» Needs work

Assigned:Crell» tim.plunkett

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

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new10.38 KB
new21.67 KB
PASSED: [[SimpleTest]]: [MySQL] 52,516 pass(es).
[ View ]

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

StatusFileSize
new11.99 KB
new33.22 KB
FAILED: [[SimpleTest]]: [MySQL] 52,492 pass(es), 1 fail(s), and 6 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new34.17 KB
PASSED: [[SimpleTest]]: [MySQL] 52,487 pass(es).
[ View ]

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.

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

StatusFileSize
new1.03 KB
new1.61 KB
new33.73 KB
PASSED: [[SimpleTest]]: [MySQL] 52,459 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

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

Title:Provide a dedicated approach for using forms in routesChange 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.

Issue tags:+#SprintWeekend

Title:Change notice: Provide a dedicated approach for using forms in routesProvide 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.

Status:Active» Fixed

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.

Status:Fixed» Needs review

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

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.

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.