Problem/Motivation

We have *a lot* of duplicated, almost identical code in our delete confirmation forms. Every entity needs to copy that and make small adjustments, we also have small differences between them, some log messages, some don't, many have slightly different strings.

Proposed resolution

Add default delete forms for config and content entities, with a trait to share the code (they have different parent classes). Allow to override the deletion confirmation message and the log behavior, but do log by default.

Use that for all entity types in core, as far as possible. In some cases, I accepted the new messages and updated tests a bit, in others I added overrides. Quite a few delete forms can be removed completely.

For language deletions, two additional changes were made, access check to avoid deleting the default language is now done in the access control handler, which removes the need for a custom check in the form. Previously, that then did a redirect, now it is simply an access denied page. Updated the tests for that. Also removed a bogus check if the language really exists after it was already loaded.

Remaining tasks

User interface changes

API changes

New classes that can be used by default or with small overrides. Usage is completely optional, no existing API is changed.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we can save 700 lines of stupid code and improve DX for defining entities
Issue priority Currently on normal, but could probably be major?
Disruption None, usage is completely optional.

Original report by @plach

In #1499596: Introduce a basic entity form controller we unified the generation of entity forms for the default create/edit operation. Now we need to to the same for the deletion/deletion confirmation forms. We probably need to introduce a generic EntityDefaultFormController and a EntityDeleteFormController which would both extend a basic EntityBaseFormController.

Files: 
CommentFileSizeAuthor
#52 entity-delete-forms-1728804-52.patch79.3 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,955 pass(es).
[ View ]
#47 entity-delete-forms-1728804-47-interdiff.txt623 bytesBerdir
#47 entity-delete-forms-1728804-47.patch79.31 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-delete-forms-1728804-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#41 entity-delete-forms-1728804-41.patch79.31 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,505 pass(es).
[ View ]
#37 entity-delete-forms-1728804-37-interdiff.txt717 bytesBerdir
#37 entity-delete-forms-1728804-37.patch79.37 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,142 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#31 entity-delete-forms-1728804-31-interdiff.txt464 bytesBerdir
#31 entity-delete-forms-1728804-31.patch79.31 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,147 pass(es).
[ View ]
#28 entity-delete-forms-1728804-28.patch79.37 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,146 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

Berdir’s picture

Title:Introduce EntityDeleteFormController and sons to handle entity deletions» Introduce (Content)EntityDeleteForm and sons to handle entity deletions
Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new46.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,155 pass(es), 186 fail(s), and 0 exception(s).
[ View ]

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.

jibran’s picture

This issue has a gender bias topic :P

Status:Needs review» Needs work

The last submitted patch, 1: entity-delete-forms-1728804-1.patch, failed testing.

plach’s picture

Title:Introduce (Content)EntityDeleteForm and sons to handle entity deletions» Introduce (Content)EntityDeleteForm and children to handle entity deletions
Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new47.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,280 pass(es), 70 fail(s), and 0 exception(s).
[ View ]
new2.42 KB

Didn't update the delete forms properly.

Status:Needs review» Needs work

The last submitted patch, 5: entity-delete-forms-1728804-5.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new62.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,354 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new20.21 KB

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

dawehner’s picture

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

Status:Needs review» Needs work

The last submitted patch, 7: entity-delete-forms-1728804-7.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new70.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,324 pass(es), 51 fail(s), and 0 exception(s).
[ View ]
new18.94 KB

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.

Berdir’s picture

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

Status:Needs review» Needs work

The last submitted patch, 10: entity-delete-forms-1728804-10.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new72.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,326 pass(es), 51 fail(s), and 0 exception(s).
[ View ]
new16.52 KB

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

Status:Needs review» Needs work

The last submitted patch, 13: entity-delete-forms-1728804-11.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new73.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,489 pass(es).
[ View ]
new2.58 KB

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

jibran’s picture

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 the entity form delete link and #1842036: [META] Convert all confirm forms already converted to new routing system to use modal dialog

Berdir’s picture

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

jibran’s picture

Status:Needs review» Reviewed & tested by the community

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

Berdir’s picture

Status:Reviewed & tested by the community» Needs review

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.

tim.plunkett’s picture

  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();
    -    }

    ?!

Berdir’s picture

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.

tim.plunkett’s picture

Use getEntity(), it's on EntityForm

Wim Leers’s picture

Status:Needs review» Postponed
Issue tags:+Needs change record, +DX (Developer Experience)

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.

Berdir’s picture

Status:Postponed» Needs review
StatusFileSize
new79.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/EntityDeleteFormTrait.php.
[ View ]
new18.95 KB

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

Status:Needs review» Needs work

The last submitted patch, 24: entity-delete-forms-1728804-24.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new79.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/language/src/Tests/LanguageListTest.php.
[ View ]
new608 bytes

See.

Status:Needs review» Needs work

The last submitted patch, 26: entity-delete-forms-1728804-26.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new79.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,146 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new907 bytes

Missed one merge conflict.

Status:Needs review» Needs work

The last submitted patch, 28: entity-delete-forms-1728804-28.patch, failed testing.

andypost’s picture

+++ 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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new79.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,147 pass(es).
[ View ]
new464 bytes

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.

Status:Needs review» Needs work

The last submitted patch, 31: entity-delete-forms-1728804-31.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Berdir’s picture

tstoeckler’s picture

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.

Berdir’s picture

Issue summary:View changes
Issue tags:-Needs issue summary update
StatusFileSize
new79.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,142 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new717 bytes

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.

Berdir’s picture

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.

tstoeckler’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 37: entity-delete-forms-1728804-37.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new79.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,505 pass(es).
[ View ]

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
+++ 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.

Wim Leers’s picture

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

andypost’s picture

Status:Reviewed & tested by the community» Needs work

for change record

Berdir’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs change record

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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?

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new79.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-delete-forms-1728804-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new623 bytes

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

dawehner’s picture

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

Berdir’s picture

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.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Damn, sometimes things are not that easy in php 5.4

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 47: entity-delete-forms-1728804-47.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new79.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,955 pass(es).
[ View ]

Reroll, trivial conflict in ShortcutDeleteForm.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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!

  • alexpott committed 9be3165 on 8.0.x
    Issue #1728804 by Berdir: Introduce (Content)EntityDeleteForm and...

Status:Fixed» Closed (fixed)

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