Resolve difference between submitForm(), submit(), and save() in EntityFormController

ok, so what would be the usual submit-form behaviour of an entity form then? I guess it would just update $form_state['entity']?

Yep, we had the submit() method to recreate an entity object based on the submitted value ans update the wrapped entity object. The idea was that every action would need the submit() and then provide a specific submission handler for its task.

We could make the submit() method final and enforce a new submit handler for each action :)

Talking to @tim.plunkett on IRC

alexpott: i think save() should be renamed submitForm (the current submitForm is empty), and we should remove submit()
alexpott: yeah and anyone doing something special can specify it with #submit like normal forms

I'm +1 to this. Three methods become 1 :)

So here's a first take on make the 3 methods one. As you can see from the patch there has been confusion about which method to use - some people preferring save() and other submit(). So bumping this to major.

This patch changes everything to submitForm() which means every save method now calls parent::submitForm(). Perhaps we still want to have a save method but we should actively discourage people from overriding the EntityFormController::submitForm() by making EntityFormController abstract and moving save() to the interface but not implementing it in EntityFormController.

So lets see what's broken :)

So it appears that there is some value in having a submitForm and save method on EntityFormController since this allow for common actions for preview and save - see CommentFormController for an example.

The issue though is that EntityConfirmFormBase and ContentEntityConfirmFormBase both extend EntityFormController and on these having the submit handler call both submitForm() and save() makes no sense. Plus current head would make it impossible to provide any default code in the save() method as it is getting called on every entity confirm form submission :)

The patch attached tries to rectify this by not calling parent::actions() in EntityConfirmFormBase and ContentEntityConfirmFormBase.

It also adds documentation indicating that if you are extending EntityFormController you probably do not need to override submitForm() but the specialised submit handler methods for the actions - delete() and save().

So it appears the EntityFormController::submitForm breaks the add view wizard.

Patch should fix remaining fails.

Update

Tagging appropiately

Tagging.

Update

12: 2022875.12.patch queued for re-testing.

Re-roll #12

Rerolled again, from #12 and fixed some spelling mistakes

New form ImageStyleFlushForm...

+++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
@@ -81,14 +81,33 @@ public function form(array $form, array &$form_state) {
+  /**
+   * {@inheritdoc}
+   *
+   * The save method makes no sense on EntityConfirmFormBase. Form submissions
+   * should be processed by overriding the submitForm method.
+   */
+  public function save(array $form, array &$form_state) {}
...
+  /**
+   * {@inheritdoc}
+   *
+   * The delete method makes no sense on EntityConfirmFormBase. Form submissions
+   * should be processed by overriding the submitForm method.
+   */
+  public function delete(array $form, array &$form_state) {}

I'm rather confused by this phpDoc. I'm not sure what to make of it as a developer reading the API documentation. It reads as a '@todo: remove me' or something.

The patch no longer applies; it should be re-rolled.

Re-roll.

Needs reroll.

Re-rolled. That wasn't too ugly for a 2 month old patch of this size, let's see what the testbot thinks.

Should be green, we still need to address #26.

Oh book ...

37: drupal_2022875_37.patch queued for re-testing.

Let's see how bad I messed up this re-roll.

Strange.

Re-roll. How are we going to get this in?

Re-roll.

// Edit: not sure if I need some more re-rolling practice or if the patch should really be 150% as big as before the re-roll.

Ugh. Will go through this tomorrow morning. I didn't notice the file size until after the upload, that's why it's even in this issue.

Forget I ever uploaded #47. This should be a better/proper re-roll.

Re-roll after #2309323: Allow #submit and #validate to be specified as methods of the form object.

Wrong merge.

Hm. Not sure whether we've arrived at a clean improvement over status quo just yet — the currently proposed patch introduces new inconsistencies:

1. +        '#validate' => array(
   +          array($this, 'validate'),
   +        ),
   +        '#submit' => array(
   +          array($this, 'submitForm'),
   +        ),
   

   Why is validate not validateForm?

2. It's no longer clear whether submitForm() performs the final operation or not:

   +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -204,12 +198,14 @@ protected function actionsElement(array $form, FormStateInterface $form_state) {
   +      '#submit' => array('::submitForm', '::save'),
   

   This submitForm() does not perform the final operation.

   +++ b/core/modules/aggregator/src/Form/FeedItemsDeleteForm.php
   @@ -40,7 +40,7 @@ public function getConfirmText() {
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
        $this->entity->deleteItems();
   

   This submitForm() does perform the final operation.

   +++ b/core/modules/comment/src/CommentForm.php
   @@ -245,7 +245,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      '#submit' => array('::submitForm', '::preview'),
   

   This submitForm() does not perform the final operation.

I was wondering about validate too.

I think it's an improvement over HEAD, we got rid of submit() and submitForm() isn't the one that is final step, but it's a step that is almost always called (operation/confirmation forms are sometimes a bit different).

Great work!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
   @@ -82,14 +82,34 @@ public function form(array $form, FormStateInterface $form_state) {
   +   * {@inheritdoc}
   +   *
   +   * The save method makes no sense on EntityConfirmFormBase. Form submissions
   +   * should be processed by overriding the submitForm method.
   +   */
   +  public function save(array $form, FormStateInterface $form_state) {}
   +
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * The delete method makes no sense on EntityConfirmFormBase. Form submissions
   +   * should be processed by overriding the submitForm method.
   +   */
   +  public function delete(array $form, FormStateInterface $form_state) {}
   
   +++ b/core/lib/Drupal/Core/Entity/EntityConfirmFormBase.php
   @@ -77,13 +77,35 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * The save method makes no sense on EntityConfirmFormBase. Form submissions
   +   * should be processed by overriding the submitForm method.
   +   */
   +  public function save(array $form, FormStateInterface $form_state) {}
   +
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * The delete method makes no sense on EntityConfirmFormBase. Form submissions
   +   * should be processed by overriding the submitForm method.
   +   */
   +  public function delete(array $form, FormStateInterface $form_state) {}
   

   This might be a usecase for final, just guessing though.

2. +++ b/core/modules/block_content/src/BlockContentForm.php
   @@ -175,23 +175,15 @@ public function form(array $form, FormStateInterface $form_state) {
   +    /** @var \Drupal\block_content\BlockContentInterface $block */
   +    $block = $this->entity;
   

   Did you considered to alternative document $this->entity itself? It could be available in more places then

3. +++ b/core/modules/system/src/Form/DateFormatFormBase.php
   @@ -173,12 +173,23 @@ public function validate(array $form, FormStateInterface $form_state) {
   +    $status = $this->entity->save();
   

   Did we considered that the parent save method returns the result of $this->entity->save() so we can easily call the parent?

4. +++ b/core/modules/views_ui/src/ViewDeleteForm.php
   --- a/core/modules/views_ui/src/ViewDuplicateForm.php
   +++ b/core/modules/views_ui/src/ViewDuplicateForm.php
   
   +++ b/core/modules/views_ui/src/ViewDuplicateForm.php
   @@ -58,23 +58,26 @@ protected function actions(array $form, FormStateInterface $form_state) {
        $actions['submit'] = array(
          '#type' => 'submit',
          '#value' => $this->t('Duplicate'),
   ...
   +   * Form submission handler for the 'clone' action.
   +   *
   +   * @param array $form
   +   *   An associative array containing the structure of the form.
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *   A reference to a keyed array containing the current state of the form.
       */
   ...
   +  public function cloneView(array $form, FormStateInterface $form_state) {
   +    $this->entity = $this->entity->createDuplicate();
        $this->entity->set('id', $form_state->getValue('id'));
        $this->entity->save();
    
        // Redirect the user to the view admin form.
        $form_state->setRedirectUrl($this->entity->urlInfo('edit-form'));
   

   Don't we call it duplicate now everywhere?

Thanks for the review.

#56:
1. Added to the interface.

#58: I would prefer to look into validate()/validateForm() in a separate issue. This has been re-rolled often enough.

#61:
1. Hm. Not sure. Maybe, but that could be messy with unit tests if someone wants to unit test (parts of) a form.
2. Yes, documented the property now, I think that's a pretty good pattern. Also noticed some bogus documentation there, there is no Next button or wizard there anywhere.
3. Yes, I thought that before as well, did that now. Not too many overrides call into the parent, but what's funny is that ImageStyleEditForm actually expected it to work like that but it didn't. And now that it actually worked, there were duplicated save messages. Cleaned that form up a bit.
4. renamed to submitForm(), like most (all?) other confirm forms.

This should fix the fails. Was a temporary state while cleaning up edit forms, no longer necessary to pass it through.

+++ b/core/modules/image/src/Form/ImageStyleEditForm.php
@@ -249,17 +236,25 @@ public function effectSave($form, FormStateInterface $form_state) {
   }
 
+
   /**
    * {@inheritdoc}
    */

Extreme nitpick: superfluous new line

I'm wondering whether we still need to address #26

Removed that. Tried to improve those descriptions.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
@@ -100,16 +100,20 @@ protected function actions(array $form, FormStateInterface $form_state) {
+   * The save() method is not used EntityConfirmFormBase. This overrides the

Hmm, this should be delete() right ?

Yep and I missed one form.

Re-roll.

Checked this twice now, let's move forward here.

Committed/pushed to 8.0.x, thanks!

Could we get a change record for this?

Submitted a draft change record at https://www.drupal.org/node/2336747

I would prefer to look into validate()/validateForm() in a separate issue. This has been re-rolled often enough.

See #2453175-59: Remove EntityFormInterface::validate() and stop using button-level validation by default in entity forms. Note: that issue title doesn't currently match the patch; not sure yet if we'll want to retitle that issue or revert to that issue's earlier direction.

Published the CR