Introduce (Content)EntityDeleteForm and children to handle entity deletions

Reviving this after a very long time.

@tstoeckler has started to work on this in #2312133: Make EntityForm::save() save the entity, I'm extracting this part from there.

Not tested yet.

This issue has a gender bias topic :P

Didn't update the delete forms properly.

Fixing tests. Either by updating the tests or be adding back the old message if they were better.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDeleteForm.php
   @@ -0,0 +1,64 @@
   +   */
   +  public function getQuestion() {
   +    return t('Are you sure you want to delete the @entity-type %label', array(
   +      '@entity-type' => $this->entity->getEntityType()->getLowercaseLabel(),
   +      '%label' => $this->entity->label(),
   +    ));
   ...
   +   * {@inheritdoc}
   +   */
   +  public function getConfirmText() {
   +    return t('Delete');
   +  }
   +
   +  /**
   +   * Returns the message to display to the user after deleting the entity.
   +   *
   +   * @return string
   +   *   The translated string of the deletion message.
   +   */
   +  public function getDeletionMessage() {
   +    return $this->t('The @entity-type %label has been deleted.', array(
   +      '@entity-type' => $this->entity->getEntityType()->getLowercaseLabel(),
   +      '%label' => $this->entity->label(),
   +    ));
   +  }
   ...
   +   * {@inheritdoc}
   +   */
   +  public function getCancelUrl() {
   +    return $this->entity->urlInfo();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $this->entity->delete();
   +    drupal_set_message($this->getDeletionMessage());
   +    $form_state->setRedirectUrl($this->getCancelUrl());
   +  }
   +
   
   +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -0,0 +1,66 @@
   +   * {@inheritdoc}
   +   */
   +  public function getQuestion() {
   +    return t('Are you sure you want to delete the @entity-type %label', array(
   +      '@entity-type' => $this->entity->getEntityType()->getLowercaseLabel(),
   +      '%label' => $this->entity->label(),
   +    ));
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getConfirmText() {
   +    return t('Delete');
   +  }
   +
   +  /**
   +   * Returns the message to display to the user after deleting the entity.
   +   *
   +   * @return string
   +   *   The translated string of the deletion message.
   +   */
   +  public function getDeletionMessage() {
   +    return $this->t('The @entity-type %label has been deleted.', array(
   +      '@entity-type' => $this->entity->getEntityType()->getLowercaseLabel(),
   +      '%label' => $this->entity->label(),
   +    ));
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCancelUrl() {
   +    return new $this->entity->urlInfo();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $this->entity->delete();
   +    drupal_set_message($this->getDeletionMessage());
   +    $form_state->setRedirectUrl($this->getCancelUrl());
   +  }
   

   Litteraly this code is the same: Luke: use traits here ... ... also maybe use the StringTranslationTrait?

2. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -106,14 +91,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        $t_args = array('%language' => $this->entity->label(), '%langcode' => $this->entity->id());
        $this->logger('language')->notice('The %language (%langcode) language has been removed.', $t_args);
   

   It is not really helpful anymore to define a variable ...

Ok, made that a trait, it is a bit strange ($this->entity doesn't exist and I can't use the string translation trait there as it is coming from the form base class already, so I just have to rely on it being there...)

Also more cleanup and simplifications. Cleaned up language delete form a lot. It had a completely bogus check if the langcode exists and gets that langcode from the entity. Also the default language check should be in the access control handler, then that is checked automatically. This also allows to remove that check from LanguageListBuilder.

Almost all but not all delete forms write a log entry if something is deleted. Not sure about standardizing that as well, the part that is a bit annoying is that we can't easily use getDeletionMessage() for that as we are not supposed to translate the message, so we have to duplicate the string I guess, in a separate method.

Thoughts about making that the default behavior?

Another thing that is weird is the redirect/cancel url. right now the default cancel url is the entity url, which is also the redirect url. but that makes no sense ;) We really need a standard link template for the list...

Ok, this is starting to get interesting, implemented the logging part.

Fixed language access and the tests there and found another delete form to clean up.

Usually this patch falls in @tim.plunkett's domain. :D
Code changes looks good maybe these two issues are also related here #2254935: Use a modal for content entity form delete links confirmation forms and #1842036: [META] Convert all confirm forms to use modal dialog

Currently working on #2401505: Add an entity collection template for lists , the last step to completely remove a bunch of custom delete forms.

@ibran: While those issues are logically related, they do not overlap in any relevant way, modal is controlled where the link is displayed, I'm only touching internals of those forms, not changing in any way how to work or look (Apart from a few minor string changes, by using the default strings in cases where I think they are actually better than what we have now).

Ok then it is RTBC for me. Pinged @tim.plunkett to eyeball the patch.

Thanks, but my plan is to get #2401505: Add an entity collection template for lists done first and then update this on top of that, to see how far we can go with this. Also want to write a change record, to announce that those classes can now be used.

1. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteFormTrait.php
   @@ -0,0 +1,79 @@
   + * This trait relies on the StringTranslationTrait and the logger method added
   + * by FormBase.
   

   Please add abstract protected methods to the trait for the methods you rely on, and you can @see there and remove this comment.

2. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteFormTrait.php
   @@ -0,0 +1,79 @@
   +    return t('Are you sure you want to delete the @entity-type %label?', array(
   

   $this->t()

3. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -85,35 +40,16 @@ public function getFormId() {
   -    // Throw a 404 when attempting to delete a non-existing language.
   -    $languages = language_list();
   -    if (!isset($languages[$langcode])) {
   -      throw new NotFoundHttpException();
   -    }
   

   ?!

Thanks.

1. Can add the method, but I can't add an abstract $this->entity (which is not actually mentioned there yet), suggestions for that?

3. see #10. That exception can not possibly ever be thrown, as we get $langcode from the already upcasted entity object.

Use getEntity(), it's on EntityForm

my plan is to get #2401505: Add an entity collection template for lists done first

Hence postponing.

Also want to write a change record

Tagging accordingly.

That got in, another 200 lines of code removed, including a bunch of deleted confirmation forms that no longer need a custom form class.

Completely untested. Probably full of PHP syntax errors :)

See.

Missed one merge conflict.

+++ b/core/modules/language/src/Tests/LanguageListTest.php
@@ -75,10 +75,10 @@ function testLanguageList() {
     $this->drupalGet('admin/config/regional/language/delete/' . $langcode);
-    $this->assertUrl(\Drupal::url('entity.configurable_language.collection', [], ['absolute' => TRUE, 'language' => $language]));
-    $this->assertText(t('The default language cannot be deleted.'), 'Failed to delete the default language.');
+    $this->assertResponse(403, 'Failed to delete the default language.');

unrelated? it tests the page urls and message

Fixed the test fails.

@andypost: It is related, the language delete form had some custom access checks and did a redirect then. I moved that to an access check, which allows us to remove that and also the custom logic in the list builder. The difference is then that trying to delete that language (the default language), is simply a 403, like any other thing that can not be deleted.

I have some ideas how we could simplify a few lines further, like supporting collection link templates that are bundle specific, for terms and shortcuts, but this is probably going far enough already.

Wow, this issue is just beautiful, I can't find any other words than that.

I read through the whole patch and only have one minor quibble, otherwise this can go RTBC IMO:

+++ b/core/modules/language/src/Form/LanguageDeleteForm.php
@@ -95,35 +32,16 @@ public function getFormId() {
+    parent::submitForm($form, $form_state);
+    $this->logger('language')->notice('The %language (%langcode) language has been removed.', array('%language' => $this->entity->label(), '%langcode' => $this->entity->id()));

Seems this will log twice, no? I think logDeletion() should be overridden instead.

Wow, this issue is just beautiful, I can't find any other words than that.

/bow

You started it :)

Yes, fixed the language delete form, I must have missed that when I added the log message.

Did not create a change record yet, as @alexpott mentioned recently, they are more for things that modules must change/adapt to. But it might still be useful to tell them, not sure.

Hehe, that wasn't meant meant to be a hidden self-compliment. :-)
I meant specifically the splitting out of the getLogMessage(), etc. into separate methods. That makes things just way more maintainable, as is proven by the language deletion form.

This is RTBC for me, but - you are right - I did write some earlier less awesome version of this, so I guess it can't hurt for someone else to confirm.

Ups, lost the change from #31, thought I had an update to date local branch. Same interdiff as #31, just applied that again.

+++ b/core/modules/responsive_image/src/Entity/ResponsiveImageMapping.php
@@ -21,7 +21,7 @@
- *       "delete" = "Drupal\responsive_image\Form\ResponsiveImageMappingDeleteForm",
+ *       "delete" = "Drupal\Core\Entity\EntityDeleteForm",

Very nice.

+270, -1004 is great. Awesome work! I think this is RTBC.

57 files changed, 270 insertions, 1004 deletions.
Magnificent. Thank you!

for change record

Checked with @alexpott, he said this does not need a change record, because we are not changing anything but add something new.

+++ b/core/modules/comment/src/Form/DeleteForm.php
@@ -40,20 +33,17 @@ public function getDescription() {
     $this->logger('content')->notice('Deleted comment @cid and its replies.', array('@cid' => $this->entity->id()));

This will be logged twice - override the log method?

Nice catch, fixed that. Checked all other submitForm() overrides again, did not find another logger call (except NodeDeleteForm, but that does not call the parent).

Great effort!!

+++ b/core/lib/Drupal/Core/Entity/EntityDeleteFormTrait.php
@@ -0,0 +1,119 @@
+  /**
+   * Translates a string to the current language or to a given language.
+   *
+   * Provided by \Drupal\Core\StringTranslation\StringTranslationTrait.
+   */
+  abstract protected function t($string, array $args = array(), array $options = array());
+

I'm curios whether we should just include the string translation trait and be done, see http://3v4l.org/J6YYi

That gave me a php strict error (which is apparently not actually visible on 3v4l.org, see http://3v4l.org/sicBo), because the part you are missing is the class c extends FormBase, which already includes that trait as well.

Damn, sometimes things are not that easy in php 5.4

Reroll, trivial conflict in ShortcutDeleteForm.

Not having 700 lines of code to maintain is huge win for reducing fragility. Also bringing all config entity deletes together might allow us to manage deleting/fixing dependent config entities. Committed 9be3165 and pushed to 8.0.x. Thanks!