Make EntityForm::save() save the entity

Here we go. This updates all config entity forms up to C, all content entity forms up to C and all content entity confirm forms. That is roughly a third of all entity forms.

Here we go. Re-rolled after #2225353: Convert $form_state to an object and provide methods like setError() and #2272481: Remove usages of watchdog() from forms, plugins and controllers.

This completes the content entity forms. I didn't touch TaxonomyForm, as it is extended by ForumForm and thus the usual add-edit pattern doesn't really work there. That is, it's totally doable, but it deserves it's own issue, as it is non-trivial and thus needs to discussed, etc. separately.

Let's see.

Here we go. This completes the rest of the forms.

I also started fixing a couple things from inspecting the test output above.

Uploading the interdiffs from 1 to 3 as well as from 3 to 5 here as I had forgotte them above. There are multiple interdiffs because of merges in between.

Finally figured the problem out. I was passing $form_state by reference all over the place but EntityForm::save() does not. PhpStorm could really be more helpful in such cases.

Let's see what's left.

Here we go. This should be green, or at least almost green. (Yes, I ran all those locally, can I get a cookie... ;-))

I found a view conversions which are in fact not necessary for this issue, I will split those out into separate issues.

Also found a problem with ViewTestBase and XDebug cookie forwarding, will open a separate issue for that as well.
So check the "related issues" block, in case you're interested in that.

Edit: Darn, I forgot -C -M when generating the patch, that's why it's so huge. So don't bother reviewing it (or at least those parts) I will post a proper patch in case this is green.

Let's see. Crossing my fingers.

Meh, run in circles much, @tstoeckler?!

Ahh, so this conflicted with #2278567: Standardize node route names by relationship.
For anyone that still doesn't believe it: This is proof that converting stuff to new APIs while in the context of fixing other stuff (or moving around code, as it is here) is very important and in no way scope-creep.

Here's a self-review now that this passes. I will now also work on a "minimal" version of this patch, which excludes all changes that are not strictly necessary, now that the approach is proven to work for all forms, without much overhead (only 1 form in core needs to override save() to opt out of the saving).

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDeleteFormBase.php
   @@ -0,0 +1,24 @@
   +abstract class ContentEntityDeleteFormBase extends ContentEntityConfirmFormBase {
   ...
   +  public function save(array $form, FormStateInterface $form_state) {
   +    $this->entity->delete();
   
   +++ b/core/lib/Drupal/Core/Entity/EntityDeleteFormBase.php
   @@ -0,0 +1,26 @@
   +abstract class EntityDeleteFormBase extends EntityConfirmFormBase {
   ...
   +  public function save(array $form, FormStateInterface $form_state) {
   +    $this->entity->delete();
   

   Let's actually override actions() to provide a proper delete() submission handler.

2. +++ b/core/modules/aggregator/src/Form/FeedDeleteForm.php
   @@ -40,8 +40,9 @@ public function getConfirmText() {
   +    ¶
   

   Boo!

3. +++ b/core/modules/block_content/src/Form/BlockContentAddForm.php
   @@ -0,0 +1,44 @@
   +    $block_type = entity_load('block_content_type', $block->bundle());
   
   +++ b/core/modules/block_content/src/Form/BlockContentEditForm.php
   @@ -0,0 +1,35 @@
   +    $block_type = entity_load('block_content_type', $block->bundle());
   

   Boo!

4. +++ b/core/modules/comment/src/Form/CommentAddForm.php
   @@ -0,0 +1,13 @@
   +<?php
   +/**
   
   +++ b/core/modules/comment/src/Form/CommentEditForm.php
   @@ -0,0 +1,13 @@
   +<?php
   +/**
   
   +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentAddForm.php
   @@ -0,0 +1,13 @@
   +<?php
   +/**
   
   +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentEditForm.php
   @@ -0,0 +1,13 @@
   +<?php
   +/**
   
   +++ b/core/modules/taxonomy/src/Form/TermAddForm.php
   @@ -0,0 +1,33 @@
   +<?php
   +/**
   
   +++ b/core/modules/taxonomy/src/Form/TermEditForm.php
   @@ -0,0 +1,33 @@
   +<?php
   +/**
   

   Missing empty line.

5. +++ b/core/modules/shortcut/src/ShortcutSetForm.php
   @@ -64,10 +64,12 @@ public function validate(array $form, FormStateInterface $form_state) {
   +    parent::save($form, $form_state);
   +
        $entity = $this->entity;
        $is_new = !$entity->getOriginalId();
   -    $entity->save();
   

   Order seems important here. It needs to be verified that this still works.

6. +++ b/core/modules/user/src/RegisterForm.php
   @@ -98,15 +98,13 @@ public function submit(array $form, FormStateInterface $form_state) {
   +    parent::save($form, $form_state);
   ...
        $account = $this->entity;
        $pass = $account->getPassword();
   ...
   -    $account->save();
   

   It needs to be verified that this still works due to the different order.

Here we go.

Next step: Provide a "minimal" patch.

Here we go. ImageStyleDeleteForm::submit() was no longer being called after we renamed the submit handlers in EntityDeleteFormBase::actions().

Thanks for the review!

1. Yeah, I was afraid someone would ask for that. Will open some follow-ups :-)

2. Noooo!!!!!!!!!! Will fix

3. Yeah, that's kind of weird. CommentForm can't be really be split as is because it doesn't provide any specialized messages or so. So I think it makes more sense to just leave those out and have both the 'add' and the 'edit' operation point to the same class (CommentForm).

Fixed #20. See the related issues for the follow-ups I opened.

Had a stab at #23, that was a really great suggestion! Let me know what you think.

Re #22: I still on plan on providing a "minimal" version of this patch, but the problem is that many forms currently check the return value of $this->entity->save() to decide whether it is adding or updating an entity. If we want to utilize parent::save() that does not work anymore. So instead of replacing one workaround with another workaround with another one I'm fixing things properly. In the "minimal" patch I will just leave as many save() functions as possible untouched.

Oops, forgot to actually add the follow-ups to the related issues. :-)

Conflicted on #1987882: Convert content_translation routes to a new style controller.

So I recently saw #2022875: Resolve difference between submitForm(), submit(), and save() in EntityFormController and that it is making a lot of the same changes as here, which demotivated a bit to work on this issue any further... :-)

Let's see if that gets in anytime soon.