Problem/Motivation

#1903176: Add a drupal_build_form() helper for using a method as the callback introduced a procedural helper method for using an object method for a form callback. It just wrapped a pattern already used in core.
However, it was just a neat trick that happened to work, and isn't something that should continue now that D8 is increasingly OO.

Proposed resolution

Introduce a FormInterface, with methods for getting the form ID, building the form, validation, and submission.
Modify drupal_prepare_form() to call the validation/submission methods in addition to the currently magic-named functions ('_validate'/'_submit')
Convert the usage of drupal_get_callback_form() to use FormInterface

Remaining tasks

N/A

User interface changes

N/A

API changes

Remove the only-recently-added drupal_get_callback_form() function, and allow drupal_get_form() to take an object instead of a form ID.

Follow-ups

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
21.83 KB

This actually felt very natural to use while converting the core code.

tim.plunkett’s picture

FileSize
2.54 KB
21.38 KB

I'm not sure that the abstract class FormBase is actually useful. It takes something like "Drupal\form_test\FormTestObject" and turns it into "drupal-form-test-formtestobject". Also, since so far in the conversions, everything but the test class extends from another base class already, it might never be used at all.

tim.plunkett’s picture

FileSize
22.13 KB
3.42 KB

chx gave me some live feedback in IRC. This looks much nicer.

tim.plunkett’s picture

+++ b/core/includes/form.incundefined
@@ -106,8 +107,9 @@
@@ -134,28 +136,11 @@ function drupal_get_form($form_id) {

@@ -134,28 +136,11 @@ function drupal_get_form($form_id) {
   array_shift($args);
   $form_state['build_info']['args'] = $args;
 
-  return drupal_build_form($form_id, $form_state);
-}
+  if (is_object($form_id) && $form_id instanceof FormInterface) {
+    $form_state['build_info']['callback_object'] = $form_id;
+    $form_id = $form_id->getFormID();

chx also suggests to change drupal_get_form($form_id) to drupal_get_form($arg) and have either $form_id = $arg or $form_id = $arg->getFormID().

However, that means we have $arg and $args in the same function.
I'll give this a rest for now and let other people review.

tim.plunkett’s picture

FileSize
3.09 KB
23.35 KB

I had a typo in drupal_prepare_form(), and I forgot to change system_config_form(). I also changed drupal_get_form($form_id) to drupal_get_form($form_arg), to distinguish it from $args.

Status: Needs review » Needs work

The last submitted patch, form-1913084-5.patch, failed testing.

dawehner’s picture

This looks quite good so far, here are some small points.

+++ b/core/includes/form.incundefined
@@ -126,36 +128,23 @@
+  if (is_object($form_arg) && $form_arg instanceof FormInterface) {

This is_object is not needed, as instanceof takes care about that.

+++ b/core/includes/form.incundefined
@@ -773,7 +762,13 @@ function drupal_retrieve_form($form_id, &$form_state) {
+    $callback = $form_state['build_info']['callback'];

In theory we could inject the form_id into a FormInterface object which uses this form_id to generate the form, so you never have to deal with something else then an object, but yeah that's maybe out of scope of that issue.

+++ b/core/modules/block/lib/Drupal/block/BlockListController.phpundefined
@@ -175,15 +185,23 @@ public function form($form, &$form_state) {
+  public function validate(array $form, array &$form_state) {
+    // No validation.
+  }

Maybe we could provide a base which doesn't do any kind of validation/submit, so you don't have to specify it?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -43,19 +43,16 @@ public function getRegions() {
-   * Overrides Drupal\field_ui\OverviewBase::form().
-   *
-   * @param array $form
-   *   An associative array containing the structure of the form.
-   * @param array $form_state
-   *   A reference to a keyed array containing the current state of the form.

It's especially great to remove all that dump documentation of $form and $form_state, at some point you just have figured that out.

chx’s picture

>This is_object is not needed, as instanceof takes care about that.

I would rather not rely on that, I asked for the is_object even if superfluous after writing http://www.phpwtf.org/instanceof-smart .

> In theory we could inject the form_id into a FormInterface object which uses this form_id to generate the form, so you never have to deal with something else then an object, but yeah that's maybe out of scope of that issue.

I do not get this one.

> Maybe we could provide a base which doesn't do any kind of validation/submit, so you don't have to specify it?

Yes, that's what I was thinking too but then I thought, maybe the classes that do this want to extend something else and providing a form is just a 'side' thing but then again it's not mandatory they extend said class.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
791 bytes
23.11 KB

I forgot to run with PHP 5.3.5, I hit https://bugs.php.net/bug.php?id=43200 #1800122: Bump minimum version of php required to 5.3.10
Though in this case there was no reason to do that.

andypost’s picture

Looks great!

+++ b/core/lib/Drupal/Core/Form/FormInterface.phpundefined
@@ -0,0 +1,56 @@
+   * Returns a unique string identifying the form.
+   *
+   * @return string
+   *   The unique string identifying the form.
+   */
+  public function getFormID();

+++ b/core/modules/block/lib/Drupal/block/BlockListController.phpundefined
@@ -92,9 +93,18 @@ protected function sort(Block $a, Block $b) {
+  public function getFormID() {
+    return 'block_admin_display_form';

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.phpundefined
@@ -32,19 +32,16 @@ public function getRegions() {
+  public function getFormID() {
+    return 'field_ui_display_overview_form';

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -43,19 +43,16 @@ public function getRegions() {
+  public function getFormID() {
+    return 'field_ui_field_overview_form';

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestObject.phpundefined
@@ -0,0 +1,52 @@
+  public function getFormID() {
+    return 'form_test_form_test_object';

The only thing that looks strange. How should it work for multiple forms per class per #1903176-19: Add a drupal_build_form() helper for using a method as the callback

chx’s picture

It won't work for multiple forms but form alter, form cache, css needs a form_id. Using the classname would look weird, wouldn't it?

Xano’s picture

+++ b/core/lib/Drupal/Core/Form/FormInterface.php
@@ -0,0 +1,56 @@
+  public function form(array $form, array &$form_state);

Can we call this method build() or construct()? The whole interface is about form handling, and one of those names more clearly reflects the method's purpose.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
9.4 KB
27.32 KB

build() brings us closer in line with EntityFormControllerInterface, which means it can now extend FormInterface, which brings us one step closer to killing the contents of entity_get_form().
I'm very happy with how this turned out, thanks @chx, @dawehner, and @Xano for your feedback!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That's awesome!!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, form-1913084-13.patch, failed testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

This is looking really cool.

+++ b/core/includes/form.incundefined
@@ -106,8 +107,9 @@
+ *   An form object to use to build the form, or the unique string identifying

A form object..

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -33,10 +33,17 @@ public function __construct($operation) {
+    // Handled by entity_form_id().

Couldn't this method either use entity_form_id or move that logic into here instead? Maybe I'm missing something, I am joining the party late.

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormObjectTest.phpundefined
@@ -0,0 +1,45 @@
+  function testFoo() {

Maybe this test method needs a better name?

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/FormTestObject.phpundefined
@@ -0,0 +1,52 @@
+ * Provides a test Form object.

form object

tim.plunkett’s picture

FileSize
23.15 KB
6.02 KB

Rerolled with @damiankloip's fixes from #16.
Unfortunately, the EntityFormController is built in such a way that it does not expect validate or submit to run for the entire form always, just per button when specified. So we can't convert that here yet anyway.

So, the interdiff is against #11, not #13. Leaving RTBC if it passes.

damiankloip’s picture

Fair enough, patch looks good.

tim.plunkett’s picture

tim.plunkett’s picture

Issue summary: View changes

Filled out issue summary

sun’s picture

+++ b/core/lib/Drupal/Core/Form/FormInterface.php
@@ -0,0 +1,56 @@
+  public function build(array $form, array &$form_state);

Hm. I really wonder whether we shouldn't name these buildForm(), validateForm(), and submitForm()...?

Otherwise, we'll probably run into a dozen of conflicts with regard to "build" and "validate" at least...

+++ b/core/lib/Drupal/Core/Form/FormInterface.php
@@ -0,0 +1,56 @@
+  public function validate(array $form, array &$form_state);
...
+  public function submit(array $form, array &$form_state);

All implementations must match the interface. $form can be taken by reference - will PHP blow up if an implementation takes it by reference while the interface declares the signature as copy?

Xano’s picture

Hm. I really wonder whether we shouldn't name these buildForm(), validateForm(), and submitForm()...?

Otherwise, we'll probably run into a dozen of conflicts with regard to "build" and "validate" at least...

I had the same concerns.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs work

Conflict with *what*? What would it possibly conflict with?
If a class is representing a single form, what else would it need those names for?

Seriously, validate and submit can take $form by reference? Ugh. I see now that form_test_validate_form_validate() does it just to show off, but I don't see any usecases. I'll fix that.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.57 KB
23.7 KB

Oh, I didn't mean to un-RTBC.

sun’s picture

On name clashes:
Well, it appears as if there's a good chance for us to make (other) classes simply implement the FormInterface, as demonstrated in this patch even; e.g., Block plugin classes, Entity[.+]Controllers, perhaps even Views handlers (who knows)... Consequently, there's a very high chance for name clashes in class methods. The additional "Form" suffix prevents those and also makes the method names more self-descriptive/self-documenting.

On &$form:
Yeah, not used in core, but in contrib. And extensively tested by Form API tests. I introduced that for D7, and it allowed me to get rid of tons of hacks in contributed modules that are operating in the form validation layer.

tim.plunkett’s picture

Actually, I originally had buildForm, validateForm, submitForm, but it was easier to convert OverviewBase and BlockListController with these names, and it leads nicely to #1913618: Convert EntityFormControllerInterface to extend FormInterface.
I also have a bunch of code locally for #1913856: Convert Views UI forms to use FormInterface, and I can attest that these names feel much more natural.

Xano’s picture

Name clashes are fairly realistic (heck, I maintain one or two entity types with at least a validate() method), unless we decide that these interfaces should only be used for form controllers rather than classes that do more than just forms.

tim.plunkett’s picture

Why would an entity type implement the FormInterface interface? Or any object that has a concept of validation and submission that wasn't already a form?

sun’s picture

Why would an entity type implement the FormInterface interface?

The question much rather is: Why wouldn't it?

tim.plunkett’s picture

Because entity types have form controllers. As I linked to before, #1913618: Convert EntityFormControllerInterface to extend FormInterface.
An object that doesn't represent a form shouldn't be a form.

tstoeckler’s picture

I guess Webform in D8 could implement FormInterface. Although it could also use a form controller I'm not sure, just wanted to bring that up.

xmacinfo’s picture

Status: Reviewed & tested by the community » Fixed

Looks like this have been committed. :-)

http://drupalcode.org/project/drupal.git/commit/b330147

yched’s picture

This looks friggin' cool DX wise !

I'm a little bit worried, though, that we put more and more objects in $form / $form_state arrays, while the nature of these arrays is that their typical lifecycle includes being serialized / unserialized.

- If I'm not mistaken, the FormInterface object that was serialized as part of $form['#validate'] and $form['#submit'] will come back as two different objects after unserialization ? i.e it won't be the same object that validates and submits ?
- Also, impact on the size of the serialized data, if all references to the same objects are expanded into duplicate serialized string chunks.

Those do not really encourage "thick" FormInterface objects (i.e a FormInterface object that is also something else, with loads of instance properties that are painful / dangerous to serialize).

sun’s picture

Multiple copies on unserialization: Righto, problem.

We once discussed that negative consequence in #1454730-15: Allow OO methods as FAPI / render API #callbacks — quite potentially we need to bump that issue to a major task.

yched’s picture

OK, I should have checked before opening my mouth.
The concerns in #32 are moot, php serialization handles those nicely, and references to a same object are preserved on unserialize.

As pointed in #1454730: Allow OO methods as FAPI / render API #callbacks, the issue is rather for objects referenced in both $form and $form_state, since those two are serialized separately, and thus won't reference the same object when unserialized.

dawehner’s picture

There is for sure a problem with serialization: Once you don't use db_query/db_select anymore but inject the database connection you can't serialize this objects anymore, which means for example that you can't inject plugin managers, as they might depend on database cache.

sharjeel’s picture

Hello everyone ...!!!

sun’s picture

See #1743686-38: Condition Plugin System for why I raised the concern of method name clashes.

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
12.81 KB

Okay, time to swallow my pride. While the current method names worked great for the 3 classes converted by this issue, it has become apparent that they are far too generic for widespread usage.

So switching from:

build() to buildForm()
validate() to validateForm()
submit() to submitForm()

sun, feel free to tell me you told me so :)

sun’s picture

@tim: I won't. I deeply respect your knowledge and expertise in this field. Sorry for my snarky, emotionally-driven comments.

#38 looks ready to fly if it comes back green. :)

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
EclipseGc’s picture

Priority: Normal » Major

Conditions API is actually working against this patch and this seems pretty no brainer. If this could get committed sooner rather than later I'd be very thankful.

Eclipse

fago’s picture

The new method names are indeed better and do not conflict so quick, so +1 for moving on with this!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Ah OK. Committed/pushed.

geerlingguy’s picture

Status: Fixed » Needs work
Issue tags: +Needs change record

Needs a change notice—and should we unpublish or delete http://drupal.org/node/1910136?

tim.plunkett’s picture

We can just take that one over I think. I'll work on this later, it's already assigned to me :)

tim.plunkett’s picture

Issue tags: +FormInterface

Tagging.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Fixed
Issue tags: -Needs change record
Xano’s picture

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

Anonymous’s picture

Issue summary: View changes

added follow-ups