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
Comment | File | Size | Author |
---|---|---|---|
#38 | form-1913084-38.patch | 12.81 KB | tim.plunkett |
#23 | form-1913084-23.patch | 23.7 KB | tim.plunkett |
#23 | interdiff.txt | 5.57 KB | tim.plunkett |
#17 | interdiff.txt | 6.02 KB | tim.plunkett |
#17 | form-1913084-17.patch | 23.15 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThis actually felt very natural to use while converting the core code.
Comment #2
tim.plunkettI'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.
Comment #3
tim.plunkettchx gave me some live feedback in IRC. This looks much nicer.
Comment #4
tim.plunkettchx 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.
Comment #5
tim.plunkettI 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.
Comment #7
dawehnerThis looks quite good so far, here are some small points.
This is_object is not needed, as instanceof takes care about that.
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.
Maybe we could provide a base which doesn't do any kind of validation/submit, so you don't have to specify it?
It's especially great to remove all that dump documentation of $form and $form_state, at some point you just have figured that out.
Comment #8
chx CreditAttribution: chx commented>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.
Comment #9
tim.plunkettI 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.
Comment #10
andypostLooks great!
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
Comment #11
chx CreditAttribution: chx commentedIt 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?
Comment #12
XanoCan 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.
Comment #13
tim.plunkettbuild() 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!
Comment #14
dawehnerThat's awesome!!
Comment #16
damiankloip CreditAttribution: damiankloip commentedThis is looking really cool.
A form object..
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.
Maybe this test method needs a better name?
form object
Comment #17
tim.plunkettRerolled 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.
Comment #18
damiankloip CreditAttribution: damiankloip commentedFair enough, patch looks good.
Comment #19
tim.plunkettOpened #1913618: Convert EntityFormControllerInterface to extend FormInterface as a follow-up.
Comment #19.0
tim.plunkettFilled out issue summary
Comment #20
sunHm. 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...
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?
Comment #21
XanoI had the same concerns.
Comment #22
tim.plunkettConflict 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.
Comment #23
tim.plunkettOh, I didn't mean to un-RTBC.
Comment #24
sunOn 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.
Comment #25
tim.plunkettActually, 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.
Comment #26
XanoName 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.
Comment #27
tim.plunkettWhy 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?
Comment #28
sunThe question much rather is: Why wouldn't it?
Comment #29
tim.plunkettBecause 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.
Comment #30
tstoecklerI 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.
Comment #31
xmacinfoLooks like this have been committed. :-)
http://drupalcode.org/project/drupal.git/commit/b330147
Comment #32
yched CreditAttribution: yched commentedThis 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).
Comment #33
sunMultiple 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.
Comment #34
yched CreditAttribution: yched commentedOK, 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.
Comment #35
dawehnerThere 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.
Comment #36
sharjeel CreditAttribution: sharjeel commentedHello everyone ...!!!
Comment #37
sunSee #1743686-38: Condition Plugin System for why I raised the concern of method name clashes.
Comment #38
tim.plunkettOkay, 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()
tobuildForm()
validate()
tovalidateForm()
submit()
tosubmitForm()
sun, feel free to tell me you told me so :)
Comment #39
sun@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. :)
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedComment #41
EclipseGc CreditAttribution: EclipseGc commentedConditions 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
Comment #42
fagoThe new method names are indeed better and do not conflict so quick, so +1 for moving on with this!
Comment #43
catchAh OK. Committed/pushed.
Comment #44
geerlingguy CreditAttribution: geerlingguy commentedNeeds a change notice—and should we unpublish or delete http://drupal.org/node/1910136?
Comment #45
tim.plunkettWe can just take that one over I think. I'll work on this later, it's already assigned to me :)
Comment #46
tim.plunkettTagging.
Comment #47
tim.plunkettI wrote http://drupal.org/node/1932058 and modified http://drupal.org/node/1910136.
Comment #48
XanoRelated: #1938992: Use EmbeddedFormInterface to embed form snippets in forms
Comment #49.0
(not verified) CreditAttribution: commentedadded follow-ups