Objective

  1. hook_forms() exists to address the following use-cases, which can be solved differently with objectified forms now:

    1. 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().

    2. 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 additional getBaseFormID() method, so as to get additional alter + theme hooks.

    3. 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 calling parent::buildForm().

Proposed resolution

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

  1. #2110953: Change record: Convert views_forms() to a classed form
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

dawehner’s picture

The reason why views uses hook_forms() is to easily provide nice unique IDs for exposed forms and other views forms.

Xano’s picture

Using FormInterface::getFormID() and class inheritance we should be able to do the same thing.

Xano’s picture

Status: Active » Needs review
FileSize
13.59 KB

This removes all references to hook_forms(), except views_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.

Xano’s picture

Issue tags: +API change

Tagging.

Xano’s picture

Issue summary: View changes

.

The last submitted patch, drupal_2110951_4.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Added API changes and dependencies.

Xano’s picture

Status: Needs work » Needs review

4: drupal_2110951_4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: drupal_2110951_4.patch, failed testing.

Xano’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
19.6 KB

Re-roll. Most changes are because the form API is now the form_builder service.

Xano’s picture

Issue summary: View changes
Status: Needs work » Needs review

Trying to set to needs review again.

Status: Needs review » Needs work

The last submitted patch, 9: drupal_2110951_9.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
1.08 KB

Status: Needs review » Needs work

The last submitted patch, 12: drupal_2110951_12.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

12: drupal_2110951_12.patch queued for re-testing.

dawehner’s picture

I 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?

Xano’s picture

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

Xano’s picture

Status: Needs work » Needs review
Xano’s picture

12: drupal_2110951_12.patch queued for re-testing.

The last submitted patch, 12: drupal_2110951_12.patch, failed testing.

Xano’s picture

FileSize
20.84 KB

Re-roll.

Xano’s picture

20: drupal_2110951_20.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 20: drupal_2110951_20.patch, failed testing.

The last submitted patch, 20: drupal_2110951_20.patch, failed testing.

Xano’s picture

FileSize
20.89 KB

Re-roll.

dawehner’s picture

Status: Needs work » Needs review

.

Xano’s picture

Thanks!

dawehner’s picture

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

sun’s picture

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

  1. Rewrite the issue summary to extensively explain why this is no longer needed and how former use-cases are covered now.
  2. Make contrib maintainers aware of this proposal (via g.d.o/core + twitter).
Xano’s picture

Issue summary: View changes
Xano’s picture

Proposed change notice

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',
  );
}

/**
 * Implements form build callback.
 */
function foo_form(array $form, array &$form_state) {
  $form['bar'] = array(
    '#title' => t('Bar'),
    '#type' => 'textfield',
  );
 
  return $form;
}
 
/**
 * Implements page callback: returns a 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;
  }

  /**
   * {@inheritdoc}
   */
  public function getFormId() {
    return 'foo_' . $this->type . '_form';
  }
}

class FooRouteController {

  public function fooBarForm($type) {
    $form = new FooForm('bar');

    return \Drupal::formBuilder()->getForm($form);
  }
}
Xano’s picture

I also tweeted about this. Please re-tweet, so we can get more people to look at this.

sun’s picture

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

This looks OK to me.

sun’s picture

Issue summary: View changes

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

sun’s picture

Issue summary: View changes
longwave’s picture

In Ubercart 8.x-4.x, hook_forms provides the add to cart form:

function uc_product_forms($form_id, $args) {
  $forms = array();
  if ($args[0] instanceof NodeInterface) {
    $product = $args[0];
    if (uc_product_is_product($product)) {
      $forms['uc_product_add_to_cart_form_' . $product->id()] = array('callback' => 'uc_product_add_to_cart_form');
    }
  }
  return $forms;
}

Other modules that provide product-like nodes can also implement hook_forms to provide their own add to cart buttons with custom functionality:

function uc_product_kit_forms($form_id, $args) {
  $forms = array();
  if ($args[0] instanceof NodeInterface) {
    $product = $args[0];
    if ($product->getType() == 'product_kit') {
      $forms['uc_product_add_to_cart_form_' . $product->id()] = array('callback' => 'uc_product_kit_add_to_cart_form');
    }
  }
  return $forms;
}

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?

sun’s picture

Thanks for your feedback, @longwave!

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?

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

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Seems like this still needs review.

Xano’s picture

Other modules that provide product-like nodes can also implement hook_forms to provide their own add to cart buttons with custom functionality:

As 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 the getFormId() method to provide a form ID of a different format than the base form, and it can implement BaseFormIdInterface 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.

Seems like this still needs review.

@longwave, @sun, have my explanations sufficiently addressed your concerns to RTBC this again?

Xano’s picture

24: drupal_2110951_24.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I think we're good to go here.

longwave’s picture

I 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 convert

/**
* Implements hook_forms().
*/
function foo_forms() {
  return array(
    'foo_form_1' => 'foo_form',
    'foo_form_2' => 'foo_form',
    'foo_form_3' => 'bar_form',
  );
}

where 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().

Xano’s picture

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

longwave’s picture

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

xjm’s picture

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

Xano’s picture

Thanks for letting us know :)

Draft change notice at https://drupal.org/node/2188851.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks! Just need to publish the change record.

Xano’s picture

The change record has been published.

Status: Fixed » Closed (fixed)

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