Objective
-
hook_forms()
exists to address the following use-cases, which can be solved differently with objectified forms now:-
Multiple variations of the same form, whereas possible variations are controlled by the module that owns the form (e.g., Node forms).
→ This can now be done by passing a custom argument to your Form class constructor and dynamically generating the form ID in
FormInterface::getFormId()
. -
Adding custom variations for a form that is not owned by the module that adds the variations.
Use-case: Especially on custom sites, this allows you to easily have additional alter + theme hooks for a particular form, so you can present it differently in a certain spot.
→ This can now be done by creating a new
class MyFooForm extends FooForm implements BaseFormIdInterface
in your module and implementing the additionalgetBaseFormID()
method, so as to get additional alter + theme hooks. -
Wrapping certain forms with a custom "wrapper callback" that is executed before the form constructor.
→ This can now be done by extending the desired form classes and executing custom code in
buildForm()
before callingparent::buildForm()
.
-
Proposed resolution
-
Remove
hook_forms()
entirely.
API changes
hook_forms()
has been removed, since the functionality can be achieved by class inheritance.
All forms that rely on hook_forms()
need to be converted to a class that implements \Drupal\Core\Form\FormInterface
, and which gets the desired form variation name injected:
Drupal 7
/**
* Implements hook_forms().
*/
function foo_forms() {
return array(
'foo_bar_form' => 'foo_form',
'foo_baz_form' => 'foo_form',
'foo_qux_form' => 'foo_form',
);
}
/**
* Form constructor for foo_form().
*/
function foo_form(array $form, array &$form_state) {
$form['bar'] = array(
'#title' => t('Bar'),
'#type' => 'textfield',
);
return $form;
}
/**
* Menu page callback: Presents the Foo form.
*/
function foo_page() {
return drupal_get_form('foo_bar_form');
}
Drupal 8
class FooForm implements FormInterface {
protected $type;
public function __construct($type) {
$this->type = $type;
}
public function getFormId() {
return 'foo_' . $this->type . '_form';
}
}
class FooRouteController {
public function fooBarForm($type) {
$form = new FooForm('bar');
return \Drupal::formBuilder()->getForm($form);
}
}
.
.
Dependencies
Comment | File | Size | Author |
---|---|---|---|
#24 | drupal_2110951_24.patch | 20.89 KB | Xano |
#20 | drupal_2110951_20.patch | 20.84 KB | Xano |
Comments
Comment #1
XanoThis depends on #2110953: Change record: Convert views_forms() to a classed form.
Comment #2
dawehnerThe reason why views uses hook_forms() is to easily provide nice unique IDs for exposed forms and other views forms.
Comment #3
XanoUsing FormInterface::getFormID() and class inheritance we should be able to do the same thing.
Comment #4
XanoThis removes all references to
hook_forms()
, exceptviews_forms()
, which is taken care of in #2110953: Change record: Convert views_forms() to a classed form. I hope I got the documentation update right.The tests should fail, as Views still depends on this until that other issue is fixed.
Comment #5
XanoTagging.
Comment #5.0
Xano.
Comment #6.0
(not verified) CreditAttribution: commentedAdded API changes and dependencies.
Comment #7
Xano4: drupal_2110951_4.patch queued for re-testing.
Comment #9
XanoRe-roll. Most changes are because the form API is now the form_builder service.
Comment #10
XanoTrying to set to needs review again.
Comment #12
XanoComment #14
Xano12: drupal_2110951_12.patch queued for re-testing.
Comment #15
dawehnerI know that people used hook_forms() for all kind of crazy shit, though the views case has shown that an object/interface approach can deal with it,
and probably can provide more features in the future.
Now that we remove the wrapper callback feature, should we not add an explicit test coverage to support such multi-forms. Do we have such a unit test already?
Comment #16
XanoThe patch removes wrapper callbacks. Their functionality has been replaced by OOP inheritance. This is a PHP feature and I do not think we should test for that.
The base form ID functionality has been replaced by
BaseFormIdInterface
and\Drupal\Tests\Core\Form\FormBuilderTest::testGetFormIdWithBaseForm()
already tests that.Comment #17
XanoComment #18
Xano12: drupal_2110951_12.patch queued for re-testing.
Comment #20
XanoRe-roll.
Comment #21
Xano20: drupal_2110951_20.patch queued for re-testing.
Comment #24
XanoRe-roll.
Comment #25
dawehner.
Comment #26
XanoThanks!
Comment #27
dawehnerWhile I am convinced that we don't need a wrapper callback anymore it would be great to get some feedback from an actual FAPI maintainer.
The code/doc changes itself look pretty good.
Comment #28
sunHm. Are we really sure that this is no longer needed in D8?
Don't get me wrong; dropping unnecessary complexity is always good. However, I'm not sure whether we aren't missing use-cases. We need to be a bit careful, since this is dropping an actual Form API feature.
I'd recommend the following:
Comment #29
XanoComment #30
XanoProposed change notice
Drupal 7:
Drupal 8:
Comment #31
XanoI also tweeted about this. Please re-tweet, so we can get more people to look at this.
Comment #32
sunHelping you a bit out here: I've rewritten the issue summary. Please edit/adjust as necessary.
Still not sure whether this is really covering all existing use-cases of hook_forms() though.
Comment #33
chx CreditAttribution: chx commentedThis looks OK to me.
Comment #34
sunFilling out the gap for the second use-case after discussion in IRC.
Let's leave this in the RTBC queue for a while, so contrib maintainers are able to chime in.
Comment #35
sunComment #36
longwaveIn Ubercart 8.x-4.x, hook_forms provides the add to cart form:
Other modules that provide product-like nodes can also implement hook_forms to provide their own add to cart buttons with custom functionality:
uc_product simply calls drupal_get_form('uc_product_add_to_cart_form_' . $node->nid) to construct the correct form, no matter which module owns the node. I don't immediately see how I can delegate to different forms in D8 as I could in D7 either with inheritance or base forms.
edit: perhaps this is solvable by converting the add to cart form to a base field, and using #2114707: Allow per-bundle overrides of field definitions to allow a different definition for different product types?
Comment #37
sunThanks for your feedback, @longwave!
If the only purpose is to add a field widget to (seemingly) arbitrary entity forms, then yes, adding a base field to respective entities/bundles appears to be a more appropriate approach.
But if that is not possible, and if we cannot resolve it, then it appears that #36 is a different incarnation of use-case (2) in the issue summary and presents a blocker for the proposed removal of hook_forms().
Comment #38
webchickSeems like this still needs review.
Comment #39
XanoAs I also explained on IRC (not to stab at you for not listening, but just so readers know this short comment doesn't mean that I did not pay much attention to your concerns), this can be solved by letting other modules extend your product form class, and passing on a form class name to
FormBuilderInterface::getForm()
, rather than a form ID. The other module's child form class can optionally override thegetFormId()
method to provide a form ID of a different format than the base form, and it can implementBaseFormIdInterface
to let form API know what base form it was derived from.I also think that part of your difficulties may be caused by the fact that you are using the node system for products instead of having a product entity type, and not necessarily by using
hook_forms()
or the Drupal 8 OOP equivalent of that.@longwave, @sun, have my explanations sufficiently addressed your concerns to RTBC this again?
Comment #40
Xano24: drupal_2110951_24.patch queued for re-testing.
Comment #41
sunI think we're good to go here.
Comment #42
longwaveI still don't see how passing a base form's class name to
FormBuilderInterface::getForm()
can delegate to a subclass of that form. Or in other words, how do I convertwhere previously I could call drupal_get_form('foo_form_' . $id) and the mapping to the correct form constructor was handled automatically? How would I declare FooForm and BarForm, be able to call drupal_get_form('FooForm', $id) and get an instance of BarForm back? Note that the caller of drupal_get_form() has no knowledge of BarForm (or any other subclasses that might possibly exist), it is only declared via hook_forms().
Comment #43
Xano@longwave: Nothing is delegated. That's how forms were processed in D7: you passed on an ID, and FAPI would find the right form builder. In D8 you pass on a FormInterface object or a class name that contains its ID. It's the other way around. If your D7 code that calls drupal_get_form() does not know about the forms, you'll have to change that in your D8 port.
Comment #44
longwaveOK, so this is a minor functionality removal for some previous use cases of hook_forms(). I'm fine with that, but it should be made clear in the change notice that some uses of hook_forms() in D7 will have to be re-architected in D8.
Comment #45
xjmReminder: you can now create a draft change record for API changes.
https://groups.drupal.org/node/402688
A change record will be required for an API change to be committed starting Feb. 14, but you're encouraged to create a draft for this issue now as well. :)
Comment #46
XanoThanks for letting us know :)
Draft change notice at https://drupal.org/node/2188851.
Comment #47
catchCommitted/pushed to 8.x, thanks! Just need to publish the change record.
Comment #48
XanoThe change record has been published.