Problem/Motivation

When deleting a node(or other entity) from the edit form clicking the Delete button triggers validation on the entity. This doesn't make sense because a entity that is going to be deleted should not have to validated. It also causes users who don't have access the content overview screen to be unable to delete entities that can't be validate for a number or reasons.

Proposed resolution

Change the delete button to a delete link(similar to cancel links) on the edit form for Entities that provide a URI(unlink entities link Vocabularies).
This link will not delete directly but go to the conformation form like the current delete button.

Remaining tasks

Only need 1 more review to be RTBC.

User interface changes

Delete button changes to link

API changes

This will happen in the EntityFormController class.

Original report by naquah

(for legacy issues whose initial post was not the issue summary)
Say I am editing a node and have cleared the Title field. But then I decide to remove the node altogether. Boom. Title field is required.

It appears that clicking the Delete button on a node form triggers node validation. This is unexpected behavior and was not happening in 5.x.

I'm thinking the '#submit' => array('node_form_delete_submit') syntax for the button is misused here, since we don't actually want the form to be submitted, we just want to go to another page.

CommentFileSizeAuthor
#158 interdiff.txt8.24 KBXano
#158 drupal_216064_158.patch27.21 KBXano
#154 interdiff.txt2.45 KBParisLiakos
#154 drupal_216064_154.patch18.71 KBParisLiakos
#152 drupal_216064_152.patch18.81 KBfloretan
#149 Image.jpg9.03 KBbleen
#148 delete-as-link.png88.4 KBeffulgentsia
#146 drupal_216064_146.patch18.63 KBXano
#142 entity.delete.142.patch18.52 KBsun
#138 interdiff.txt5.39 KBsun
#138 entity.delete.138.patch18.52 KBsun
#137 interdiff.txt1.77 KBXano
#137 drupal_216064_137.patch18.36 KBXano
#135 interdiff.txt756 bytesXano
#135 drupal_216064_135.patch18.69 KBXano
#133 interdiff.txt1.19 KBXano
#133 drupal_216064_133.patch18.73 KBXano
#130 interdiff.txt1.7 KBXano
#130 drupal_216064_130.patch18.39 KBXano
#127 interdiff.txt7.5 KBXano
#127 drupal_216064_127.patch16.97 KBXano
#125 drupal_216064_125.patch8.86 KBXano
#125 interdiff.txt4.2 KBXano
#122 drupal_216064_122.patch4.24 KBXano
#119 drupal_216064_119.patch4.13 KBXano
#114 drupal-delete-button-to-link-216064-114.patch7.19 KBtedbow
#109 drupal-delete-button-to-link-216064-109.patch6.36 KBTravisCarden
#100 delete_button_to_link-216064-100.patch6.44 KBidflood
#97 216064-97.patch6.5 KBstar-szr
#97 interdiff.txt3.46 KBstar-szr
#85 delete_button_to_link-216064-85.patch6.47 KBtedbow
#82 delete_button_to_link-216064-82.patch4.2 KBtedbow
#76 216064-76-test.patch5.76 KBdrifter
#76 216064-76-combined.patch9.11 KBdrifter
#73 216064-73-test.patch5.79 KBdrifter
#73 216064-73-combined.patch9.13 KBdrifter
#71 216064-68-test.patch5.79 KBdrifter
#71 216064-68-combined.patch9.08 KBdrifter
#68 216064-68-test.patch5.79 KBdrifter
#68 216064-68-fix.patch3.34 KBdrifter
#66 216064-66-test.patch5.76 KBdrifter
#66 216064-66-fix.patch3.29 KBdrifter
#64 216064-form_validation-c64.patch10.02 KBArtusamak
#62 216064-form_validation-c62.patch9.8 KBArtusamak
#53 delete_validation-216064-45.patch9.59 KBArtusamak
#52 delete_validation-216064-45.patch9.59 KBArtusamak
#45 delete_validation-216064-45.patch9.59 KBmarcingy
#39 delete_validation-216064-39.patch9.59 KBidflood
#38 delete_validation-216064-38.patch9.59 KBidflood
#35 delete_validation_path-216064-35.patch2.11 KBidflood
#35 delete_validation_user-216064-35.patch1.5 KBidflood
#35 delete_validation_node-216064-35.patch2.4 KBidflood
#35 delete_validation_menu-216064-35.patch3.65 KBidflood
#34 delete_validation_menu-216064-34.patch3.65 KBidflood
#34 delete_validation_node-216064-34.patch2.4 KBidflood
#34 delete_validation_path-216064-34.patch2.11 KBidflood
#34 delete_validation_user-216064-34.patch1.5 KBidflood
#32 drupal-delete-validation-216064-32.patch9.6 KBidflood
#29 delete_path.patch2.41 KBygerasimov
#29 delete_menu.patch2.6 KBygerasimov
#29 delete_user.patch1.82 KBygerasimov
#29 delete_node.patch2.72 KBygerasimov
#24 forms_delete_buttons.patch2.41 KBygerasimov
#2 delete.patch846 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

decafdennis’s picture

Since the only thing the Delete button does is redirect to node/%/delete, it would be better if it were a simple link. Note that the Delete button on vocabulary forms still works by checking $_POST['op'] before the form is rendered, which is quite ugly.

Maybe there should be a way to keep (i.e. not replace them by links) the Delete buttons, not have form validation occur when clicking them, and not have the ugly $_POST['op'] == t('Delete') check. I'm thinking something like:

$form['buttons']['delete'] = array(
  '#type' => 'button', // This is a button, since we're not talking submission here
  '#value' => t('Delete'),
  '#weight' => 15,
  '#redirect' => array('node/'. $node->nid .'/delete', $destination), // Similar to #validate and #submit for submit buttons
);

So my proposal is that we let _form_builder_handle_input_element check for the presence of #redirect when a button is clicked, save this fact in the $form_state, then let drupal_process_form call drupal_redirect_form before even drupal_validate_form is called.

catch’s picture

Title: Delete button triggers node validation » Regression: Delete button triggers node validation
Version: 6.0-rc2 » 6.x-dev
Status: Active » Needs review
FileSize
846 bytes

I can see how this'd be really, really annoying.

Rolled the above into a patch, untested.

GreenLED’s picture

Ran the patch.

Before:
1. Created a node with content in both the title and body fields.
2. Click through to Administer > Manage > Content > Edit Page
3. Cleared text form the Title field.
4. Clicked "Delete"
5. Received message...

Title field is required.

After:
1. Patch ran clean, without zero errors.
2. Click through to Administer > Manage > Content > Edit Page
3. Immediately page displays error above "Title" field.

notice: Undefined variable: destination in C:\server\pub\test\6p00dev\modules\node\node.pages.inc on line 248.

Confirmation:
1. Reversed patch, above error disappeared.

» Respectfully, GreenLED
» Stable Files . net

GreenLED’s picture

Changing status.

» Respectfully, GreenLED
» Stable Files . net

decafdennis’s picture

Status: Needs review » Needs work

Quick note: the patch will need to implement the #redirect functionality for buttons too before this would work... and setting $destination to drupal_get_destination() would fix the notice.

perforator’s picture

Hi.

Is there any progress on this issue? In Drupal 6.3 it's obviously not fixed.

AltaVida’s picture

I just wanted to add, that this problem is not unique to deleting nodes. It also occurs when deleting users (see http://drupal.org/node/274547 ). Perhaps this issue should be marked as a forms API issue and not just specific to the node system.

I haven't tested but I imagine this might also occur in other areas (like deleting taxonomy terms perhaps?).

nextdude’s picture

Version: 6.x-dev » 6.9

This issue is still happening in 6.9. Is anyone still working on this?

coltrane’s picture

Work is happening on this to an effect for 7 at #370823: Allow Nodes to Be Previewed Without Required Fields.

kenorb’s picture

Got the same problem.

Pasqualle’s picture

Title: Regression: Delete button triggers node validation » Delete button triggers form validation
Version: 6.9 » 7.x-dev
Component: node system » forms system

it applies at least for the node and for the user form..
this issue is dependent on #370537: Allow suppress of form errors (hack to make "more" buttons work properly)

Aren Cambre’s picture

sun’s picture

gpk’s picture

Status: Needs work » Active

#370537: Allow suppress of form errors (hack to make "more" buttons work properly) has landed and is now at "needs work" status for the addition of docs. Should therefore be possible to fix this issue now. :D

Marking as "active" (no patch) since #2 is not relevant to 7.x.

This bit one of about 4 serious users on a site I run - we've made the node "Log message" field required like on d.o, and she couldn't work out how to delete nodes. Having to put something in the Log message box is pretty bizarre given that it will be nuked in just a moment!!!

effulgentsia’s picture

#370537: Allow suppress of form errors (hack to make "more" buttons work properly) has landed...Should therefore be possible to fix this issue now.

Yes. Just set '#limit_validation_errors' => array() on the "delete" button. Would someone like to roll that patch for all the relevant "delete" buttons in core, so it can be discussed? Not sure if it can make it into D7, but a concrete patch could help spur discussion.

gpk’s picture

thekevinday’s picture

subscribing

matt2000’s picture

This has often annoyed me too.

raimondious’s picture

This behavior is also apparent when deleting menus. A module created a menu with a name longer than 27 characters, but now I can't delete it because it tells me that the menu name is too long. Destructive actions should skip the validation step.

mitchmac’s picture

subscribing

StevenPatz’s picture

Is 27 still the max for a menu? I am trying to recreate this and I am using menu title's with more than 27 characters and they are being saved.
EDIT: Nevermind it appears to be 128 now. And this is still an issue.

ryumkin’s picture

Version: 7.x-dev » 6.19

This is realy looking strange: you can't delete the node without validation proccess.
I've tried the above solutions and they didn't help...
Subscribing.

effulgentsia’s picture

Version: 6.19 » 7.x-dev

Fixes need to be done first on the version in development (currently, D7), then only after that is complete, possibly backported to an already released version (e.g., D6).

ygerasimov’s picture

FileSize
2.41 KB

I have tired to fix the forms with '#limit_validation_errors'.

Forms that are fixed:
1. Menu edit form. admin/structure/menu/item/%/edit
2. Role edit form. admin/people/permissions/roles/edit/%
3. Node edit form. node/%/edit
4. URL alias edit form. admin/config/search/path/edit/%

Forms that cannot be fixed in this way (not fixed with this patch):
1. Content type edit form admin/structure/types/manage/%
2. Taxonomy vocabulary edit form admin/structure/taxonomy/%/edit
3. Taxonomy term edit form taxonomy/term/%/edit

Please check if I missed any other forms and whether this patch fixes above mentioned forms.

ygerasimov’s picture

Status: Active » Needs review
chx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This is one correct patch, although it needs comments ( why it's not empty? Why nid?) i do not have the shortest idea how it will be backported. Also, needs tests.

ygerasimov’s picture

Assigned: Unassigned » ygerasimov

Alright. Lets concentrate on first four forms from #24 first. I will write tests and put comments.

chx’s picture

Oh, by the way why we can't fix those three? if there is no #type value to hang your hat on, well, add one.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
1.82 KB
2.6 KB
2.41 KB

I have splitted the patch to four pieces for each form separately.

Regarding the last three forms, they cannot be fixed with #limit_validation_errors as Delete buttons don't have #submit property. So they should be fixed in different way.

Please review in the meantime patches for fixable forms.

ygerasimov’s picture

#29: delete_menu.patch queued for re-testing.

idflood’s picture

User and node apply with offset but the patch looks good to me. I tested the 4 scenarios without patches to reproduce the issue and then applied them. Everything works as expected but i noticed one other form where this validation issue occurs, admin/structure/menu/manage/%/edit ( to delete a custom menu ).

Maybe it's better to open another issue for the 3 "special" forms about taxonomy and content type.

idflood’s picture

Status: Needs review » Needs work
FileSize
9.6 KB

I worked on the "delete menu" form. I'm not sure about the new test, and if it should be moved in a new function, maybe something like $this->deleteCustomMenuValidation().

This include the 4 patches from #29 + fix for the "delete menu" form.

idflood’s picture

Status: Needs work » Needs review
idflood’s picture

Let's do it for d8 first. Patches should be identical as the previous one but I wasn't able to apply it so I made the change manually.

idflood’s picture

Status: Needs review » Needs work

The last submitted patch, delete_validation_menu-216064-35.patch, failed testing.

sun’s picture

@idflood: Please merge those patches into a single one. (just like the one in #32)

idflood’s picture

Status: Needs work » Needs review
FileSize
9.59 KB

@sun: here it is. I didn't looked at those failed tests yet.

idflood’s picture

The fix was not in the right place for user.admin.inc. This sould fix at least 1 test.

Status: Needs review » Needs work

The last submitted patch, delete_validation-216064-39.patch, failed testing.

DamienMcKenna’s picture

Just ran into this.

marcingy’s picture

Assigned: ygerasimov » marcingy

Grabing I had a more exacting patch for nodes will work on this over the next couple of days

marcingy’s picture

Status: Needs work » Reviewed & tested by the community

Looks good

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks like a mis-post.

marcingy’s picture

Status: Needs work » Needs review
FileSize
9.59 KB

There was a typo in the patch - reroll.

attiks’s picture

Artusamak’s picture

Status: Needs review » Reviewed & tested by the community

Yay, i tested all of this and it's working great, this patch is just what we need.
Curious that it hasn't been fixed for D7, it could easily be backported.

marcingy’s picture

Issue tags: +Needs backport to D7

Adding backport tag

higherform’s picture

Issue tags: +Needs backport to D6

I propose this also be backported to D6 as well, so I am tagging it as such. If for no other reason, than just because this was originally a 6.x bug!

marcingy’s picture

Drupal 6 has no concept of validation limiting so it would be an API change and as result it can't really be backported to drupal 6 is the assumption I'm making.

marcingy’s picture

Issue tags: -Needs tests

It has tests now so removing that tag.

Artusamak’s picture

The patch in #45 is also appliable to D7. Let's attach the patch for review.

Artusamak’s picture

Version: 8.x-dev » 7.x-dev
FileSize
9.59 KB

Sorry, rerolling with 7.x

marcingy’s picture

Version: 7.x-dev » 8.x-dev

Please leave at d8 as it needs to be fixed there first

Artusamak’s picture

The 7.x-dev version modification was to tell the bot to roll the patch for D7.

pjcdawkins’s picture

Subscribe

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/menu/menu.test
@@ -60,6 +60,24 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Title field is required.'), t('Delete menu on edit form with empty title.'));

@@ -72,6 +90,35 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Menu link title field is required.'), t('Delete link on edit form with empt    y title.'));
...
+    $this->assertNoText(t('Path field is required.'), t('Delete link on edit form with empty path.'));

+++ b/modules/node/node.test
@@ -394,6 +394,54 @@ class PageEditTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Title field is required.'), t('Delete button works with empty title on edit form.'));

+++ b/modules/path/path.test
@@ -107,6 +107,41 @@ class PathTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Existing system path field is required.'), t('Delete alias on edit form with empty source field.'));
...
+    $this->assertNoText(t('Path alias field is required.'), t('Delete alias on edit form with empty alias field.'));

+++ b/modules/user/user.test
@@ -1841,6 +1841,11 @@ class UserRoleAdminTestCase extends DrupalWebTestCase {
+    $this->assertNoText(t('Role name field is required.'), t('Role can be deleted with empty name submitted to the form.'));

1) Either use the proper t() source string here ("!name field is required."), or don't use t() at all.

2) Odd/stray spacing in one of the assertion messages.

+++ b/modules/node/node.test
@@ -394,6 +394,54 @@ class PageEditTestCase extends DrupalWebTestCase {
+class NodeDeleteTestCase extends DrupalWebTestCase {

Missing phpDoc.

20 days to next Drupal core point release.

pwaterz’s picture

This is also a problem on #ajax, I can't seem to stop the button from validating the form on the ajax callback.

xgougeon’s picture

Running fresh D7 :
When I try to delete taxonomy vocabulary (including taxonomy menu module)... The window-name changes to "are you sure you want to delete...blabla", but the admin overlay does not change aspect... and there is no way to go out from this loop. If I go to the homepage and back to admin, I simplu see that the vocabulary has not been deleted...
I suppose this is just a problem due to the fact that I the delete confirmation window is not well shown...
HELP !!! It's a big bug !

pjcdawkins’s picture

Hi, @xgougeon, welcome to drupal.org. It seems that your problem isn't related to this thread. You would probably be better off looking in the Taxonomy Menu issue queue (perhaps you could create a new issue there). More information on Drupal issues can be found in the issue queue handbook.

xgougeon’s picture

Hi pjcdawkins. Thanks for your help, and sorry about that I was dealing with several problems at once and got confused...

Artusamak’s picture

Assigned: marcingy » Unassigned
Status: Needs work » Needs review
FileSize
9.8 KB

Let's try that again!

attiks’s picture

Status: Needs review » Needs work

Now there's a mix of using t()

+++ b/core/modules/menu/menu.testundefined
@@ -62,6 +62,21 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertNoText('Title field is required.', t('Delete menu on edit form with empty title.'));

one string with and the other without t()

+++ b/core/modules/menu/menu.testundefined
@@ -74,6 +89,35 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertNoText('Menu link title field is required.', t('Delete link on edit form with empty title.'));

dito

+++ b/core/modules/menu/menu.testundefined
@@ -74,6 +89,35 @@ class MenuTestCase extends DrupalWebTestCase {
+    $this->assertNoText('Path field is required.', t('Delete link on edit form with empty path.'));

dito

+++ b/core/modules/node/node.testundefined
@@ -413,6 +413,59 @@ class PageEditTestCase extends NodeWebTestCase {
+    $this->assertNoText('Title field is required.', t('Delete button works with empty title on edit form.'));

dito

+++ b/core/modules/path/path.testundefined
@@ -131,6 +131,41 @@ class PathAliasTestCase extends PathTestCase {
+    $this->assertNoText('Existing system path field is required.', t('Delete alias on edit form with empty source field.'));
+

dito

+++ b/core/modules/path/path.testundefined
@@ -131,6 +131,41 @@ class PathAliasTestCase extends PathTestCase {
+    $this->assertNoText('Path alias field is required.', t('Delete alias on edit form with empty alias field.'));

dito

+++ b/core/modules/user/user.testundefined
@@ -1923,6 +1923,11 @@ class UserRoleAdminTestCase extends DrupalWebTestCase {
+    $this->assertNoText('Role name field is required.', t('Role can be deleted with empty name submitted to the form.'));

dito

Artusamak’s picture

Status: Needs work » Needs review
FileSize
10.02 KB

Reposting with the appropriate fix.

Artusamak’s picture

Assigned: Unassigned » Artusamak
Status: Needs review » Needs work

Feedback from XJM:

[17:54] Artusamak: couple thoughts on that patch. one, it is helpful to include an interdiff when you update patches
[17:54] interdiff?
[17:54] For instructions on creating an interdiff, see http://drupal.org/node/1488712 Or, if you're into microbranching like I am, see xjm's piece http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...
[17:54] Artusamak: also, you had the translations for assertions backwards
[17:54] assert t?
[17:54] Do not use t() on assertion message texts. See http://drupal.org/simpletest-tutorial-drupal7#t and http://drupal.org/node/500866.
[17:54] Artusamak: ^ review that for when it should be used and not in tests
[17:55] Artusamak: basically, it is definitely never needed on the assertion message argument, but should be used when testing *FOR* strings already translated
[17:56] Artusamak: another comment without having looked too closely is that the inline comments are wrapping very early. they should go as close to 80 chars as possible without going over. Also, it's good to use full sentences, including words like "the"; "menu link item ID" instead of mlid; etc.
[17:57] Artusamak: also, this is a really pedantic thing and I'm sorry--but all the one-line summaries in the docblocks should begin with a third-person verb
[17:58] Artusamak: for example, I would suggest changing "Delete alias through alias edit form" to "Tests deletion of a path alias through the alias edit form."
[17:59] Artusamak: and apply that sort of change to all the comments throughout the patch so that they are more complete English sentences (and less terse)
[18:01] Artusamak: oh and finally, it is also good to upload a test-only patch (as the first attachment) and the complete patch (as the second) to expose the failures of the test on the testbot. using that order prevents the bot from marking the issue needs work, also. :)

drifter’s picture

Assigned: Artusamak » drifter
Status: Needs work » Needs review
FileSize
3.29 KB
5.76 KB

Rerolling, updated tests for new locations, and the node form to EntityFormController. As a bonus, we'll get this fix for all entities using EntityFormController.

Status: Needs review » Needs work

The last submitted patch, 216064-66-fix.patch, failed testing.

drifter’s picture

Status: Needs work » Needs review
FileSize
3.34 KB
5.79 KB

A better way to determine the entity type in the EntityFormController, plus fixes for role deletion.

Status: Needs review » Needs work

The last submitted patch, 216064-68-fix.patch, failed testing.

star-szr’s picture

@drifter - Thanks for the reroll and subsequent changes! Can you please upload the test-only patch along with a patch containing both the test AND the fix? See #13 in http://drupal.org/node/1468170 for more explanation.

drifter’s picture

Status: Needs work » Needs review
FileSize
9.08 KB
5.79 KB

OK, let's try that... the failing tests passed for me locally.

Status: Needs review » Needs work

The last submitted patch, 216064-68-combined.patch, failed testing.

drifter’s picture

Status: Needs work » Needs review
FileSize
9.13 KB
5.79 KB

Hmm, didn't do my merges correctly. Sorry about that.

star-szr’s picture

Thanks @drifter! To move this even further along, there are a few minor code conventions that can be cleaned up:

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -132,6 +135,8 @@ protected function actions(array $form, array &$form_state) {
+        '#limit_validation_errors' => array(array($info['entity keys']['id'])),        ¶

Trailing whitespace here.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeDeletionTest.phpundefined
@@ -0,0 +1,61 @@
\ No newline at end of file

Add a newline (blank line) at the end of this new test file.

The inline comments should also be wrapped at 80 characters (some are over 80 characters, some wrap early) and be complete sentences as per http://drupal.org/node/1354.

star-szr’s picture

#1342444: Do not validate fields if entity is in the process of being deleted was closed as a duplicate of this issue, looks like it came to a similar conclusion anyway (modify EntityFormController).

drifter’s picture

Thanks for the review. Fixing up the comments and whitespace.

tedbow’s picture

Status: Needs review » Needs work

@drifter, went to check the patch but it not longer applies. Can you reroll?

Also I think you should be using $entity->id() to get the id of the entity in EntityFormController->actions() instead of entity_get_info. It's cleaner and simplier.

Here is the example

  protected function actions(array $form, array &$form_state) {
    $entity = $this->getEntity($form_state);
    return array(
     .....
      'delete' => array(
        '#value' => t('Delete'),
        // No need to validate the form when deleting the entity.
        // Keep the entity id key for passing an id to delete confirmation form
        '#limit_validation_errors' => array(array($entity->id())),
     
  }

Also I think we are going to need to deal with the required tags on client-side. Basically currently the browser will not let me submit the form because the browser(newer ones at least. I checked Mac Chrome) is not going to let you submit the form because of "required" tag on the input element. This happens on the client side it stops it before it actually submits it. So it is independent of the form_api.

I am not a front-end so I don't know the preferred way to deal with this(or if we should) . My guess would be through javascript but there may be a better way through an attribute.

It might be a larger issue for the form api to remove the required tags on input if the element has the '#limit_validation_errors'

agentrickard’s picture

Easier solution, of course, is to move "delete" to a separate callback / page tab. Maybe turn the button into a link.

tedbow’s picture

A link makes sense to.

If we did a separate callback wouldn't still have the client side required validation issue?
Or would it be a separate
element?

fuzzy76’s picture

Delete should really be a separate callback. The form contents have nothing to do with the delete operation, so sending it on delete does not make much sense.

But it should NOT be a link. According to the HTTP spec, potential destructive operations should never be sent through a GET request (simplified explanation), it should be used for information retrieval only. Some more info here: http://stackoverflow.com/questions/786070/why-should-you-delete-using-an...

jcisio’s picture

@fuzzy76 it will be a link to the confirmation page (which is a form with only the $entity->id() element), so no problem on GET.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.2 KB

Ok here is patch that changes the Delete button to a link in certain entity types.

The link is used for entity types that have URI.

For entity types like Vocabulary that don't provide a URI then the button is still used. In the case of the VocabularyFormController the delete action is actually a different output of the add/edit form(i.e there is not separate delete confirm form).

This will still need work and will need tests but setting needs review to get for eyes on it.

I have manually tested it with Vocabularies, Terms, Nodes

Status: Needs review » Needs work

The last submitted patch, delete_button_to_link-216064-82.patch, failed testing.

tedbow’s picture

Assigned: drifter » tedbow

@drifter I am assigning this to myself for now.

I am going to work on changing tests reflect this new change.

Still waiting on hearing back from people about this approach.

tedbow’s picture

Title: Delete button triggers form validation » Delete button triggers form validation - change to Delete link for entities with URI
Status: Needs work » Needs review
FileSize
6.47 KB

Ok, I have added some changes to tests. Hopefully this will pass all.

Changing title to reflect the fix

tedbow’s picture

Ok so it passed all the tests. Any reviews would be good.

I think it should be ok as a link because:

  • It is just a redirection to the delete confirm form which actually does the deleting.
  • Adding in a link type under the "actions" is done other places in drupal core in the same manner - confirm_form for instance.
  • It avoids all client and server side validation of the entity being deleted(should not have to validate on delete)

Hopefully we can get this in before feature freeze.

agentrickard’s picture

I like it. It is a minor, minor UI change (but smaller than turning DELETE into a tab).

If we want to use CSS to clean this up to look like a button, we can do it later.

sun’s picture

agentrickard’s picture

I'd rather not delay a long-delayed older issue in favor of a newer problem.

tedbow’s picture

I'd rather not delay a long-delayed older issue in favor of a newer problem.

I agree. The user perspective, if css is used to make it look like a button, there is not much difference to the user.
But does fix a very basic problem.

gpk’s picture

Agree, this doesn't get in the way of #1834002: Configuration delete operations are all over the place which is rather different sort of UX bug. Fixing this for 8.x might also allow the problem to be considered further for 7.x. I had to implement a custom workaround on a 6.x site a couple of years back as users couldn't work out how to delete a node with a required field on the node edit form. I see there is now http://drupal.org/project/skip_validation for 6.x and http://drupal.org/project/soft_delete for 7.x but I'm not sure that either of those quite fits my basic requirement.

tedbow’s picture

I had to implement a custom workaround on a 6.x site a couple of years back as users couldn't work out how to delete a node with a required field on the node edit form.

I think the problem that this patch solves is more serious than this.

There are some cases where without this patch a user is not able delete an entity they should be able to delete.
Sometimes an existing entity is not able to be validated at all.
A simple example of this is a node that has a required term reference field using a select list with no terms in the vocabulary.

When adding contributed modules this can happen in a number of ways.
If you have an Entity with a required Entity Reference field where the available options are controlled by a View. If the View doesn't return any results(this may be true for only some entities b/c of contextual filters) then you will not be able to delete the Entity.

If the current user doesn't have access to the Content Overview screen then there will literally be no way for the user to delete the entity though according to how the permissions are set up on the site they should be able to.

This is a problem with the core Entity system not any core or contributed fields.

I agree that #1834002: Configuration delete operations are all over the place is important but as far as I can tell it does not stop a user from performing actions that they should be able to. I think the current issue should be resolved first because it fixes a bug in Drupal that affects functionality.

agentrickard’s picture

Can we get one more review on the merits of the code in this patch and then RTBC, please.

attiks’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -132,16 +130,32 @@ protected function actions(array $form, array &$form_state) {
+        // @todo Is this the proper why to get the #href???
+        $actions['delete'] = array(
+            '#type' => 'link',
+            '#title' => t('Delete'),
+            '#href' => $entity_uri['path'] . '/delete',

is this always going to be '/delete'?

tedbow’s picture

is this always going to be '/delete'?

I think this true for all core entities that provide a URI(Vocabs don't) but of course not necessarily for contrib entities that use the EntityFormController class. Of course they can override the "actions" or "actionsElement" functions to change this.

There would also be a problem if left the delete button in place. Not all entities would handle the delete action in the same callback. So basically either way contrib modules that are using EntityFormController would have to know the default behaviour and either program to match or override 1 of these functions.

agentrickard’s picture

It should always be 'delete'. The issue that @sun links to in #88 should be a follow-up to enforce that standard.

star-szr’s picture

FileSize
3.46 KB
6.5 KB

Minor docs cleanup and a couple indentation and whitespace fixes.

tedbow’s picture

@Cottser
Thanks for that!

tedbow’s picture

Would be nice if this could get RTBC!

Since this is a "bug" and not a new feature can it be worked on after Feature Freeze?

idflood’s picture

Reroll of patch in #97 but removed the @todo since #96 seems to confirm it is correct.

// @todo Is this the proper way to get the #href?

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -Needs backport to D7

The last submitted patch, delete_button_to_link-216064-100.patch, failed testing.

idflood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, delete_button_to_link-216064-100.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
jibran’s picture

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D6, +Needs backport to D7

The last submitted patch, delete_button_to_link-216064-100.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

TravisCarden’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.36 KB
deastlack’s picture

subscribe

TravisCarden’s picture

tedbow’s picture

Status: Needs review » Needs work

@TravisCarden thanks for the re-roll.

This works for me for Nodes and Terms but not for Vocabularies

Here is the problem(thought I don't know about the solution)

$entity_uri = $entity->uri();
    if (!$entity->isNew()) {
      if (empty($entity_uri['path'])) {
        // For entities without a URI we have to use a submit button.
        $actions['delete'] = array(
          '#value' => t('Delete'),
          '#submit' => array(
            array($this, 'delete'),
          ),
        );
      }
      else {
        // For entities with a URI we provide a link to the delete form.
        $actions['delete'] = array(
          '#type' => 'link',
          '#title' => t('Delete'),
          '#href' => $entity_uri['path'] . '/delete',
        );
      }

When I wrote the patch initially "empty($entity_uri['path'])" was true for vocabularies and other entities that didn't have a separate path for deleting the entity. Now "$entity_uri['path']" has a value(which makes sense). So the current code tries to make the link to a delete path. But the Vocabulary entities don't have a separate delete path, it is just callback to the same form. So the link gets a page not found.

The problem is that I am not sure there is way to tell in code which Entity types are going to have a separate path to delete vs just a form callback.

Couple ideas on solutions:

1. We change EntityFormController->actions() to always make "delete" a link then in VocabularyFormController->actions() we could add special logic that change delete to a button. This would have to be done for any subclass of EntityFormController were the delete doesn't have a separate path.

2. We could add a function EntityFormController->deleteMethod() (or a protected variable) that would default to 'link'. Then VocabularyFormController would have to override this to "callback" . This would have to be done for any subclass of EntityFormController were the delete doesn't have a separate path.

Any other ideas?

tedbow’s picture

1 other thing I noticed(but probably should be new issue to resolve)
Drupal not puts a property on the delete button(b/c button without this patch)
$element['delete']['#button_type'] = 'danger';
As a button this puts the classes "button button-danger" on the input.
Which then moves the link over to the right of the screen(I guess because this is a destructive action)

This patch leaves that code but since it is now a link the classes don't get applied. I found that if manually add the class "button-danger" it does move the link to the right. Should this link also somehow be moved to the right?

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.19 KB

Ok, here is patch that implements by solution 2 from #112.
It takes away reliance on $entity->uri() and adds

  /**
   * The method which the form conroller uses for deleting entities
   *
   * Subclasses can change the value to "callback" if deletions are not done
   * via a link to separate form.
   *
   * @var string
   */
  protected $delete_method = 'link';

This then determines in EntityFormController->actions() whether to create a link or a button.

Status: Needs review » Needs work

The last submitted patch, drupal-delete-button-to-link-216064-114.patch, failed testing.

tedbow’s picture

So was expecting the patch in #114 to fail. There are tons of test for "Found Delete" button. Obviously these would need to be change but again want to get some feedback on the solution before we re-write tests.

Also other subclasses of "EntityFormController" would need to set $delete_method

sun’s picture

Title: Delete button triggers form validation - change to Delete link for entities with URI » Delete button triggers server-side + HTML5 form validation; change Delete button to a link
Issue tags: -Needs backport to D6, -Needs backport to D7 +Entity system, +html5

Clarifying the issue title.

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -23,6 +23,16 @@ class EntityFormController implements EntityFormControllerInterface {
+  protected $delete_method = 'link';

Given that browsers will not allow to execute a button, I do not think that it makes sense to make this switchable. We should simply use Delete links everywhere.

And yes, with the recently refactored CSS styles for buttons and link-buttons, we need to apply the two classes "button" and "button-danger" to all Delete links.

Furthermore, we'd ideally leverage the new dialog facility for links, so that the delete confirmation form appears in a modal dialog.

All of this should happen by default on the EntityFormController.

VocabularyFormController has to be changed to not embed the delete confirmation form into the regular entity form. It should use a Delete link, too.

Lastly, all tests need to be updated accordingly. Instead of drupalPost(), they need to use drupalGet() + clickLink().

dcam’s picture

dcam’s picture

Issue summary: View changes

add summary

Xano’s picture

Title: Delete button triggers server-side + HTML5 form validation; change Delete button to a link » Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link
Component: forms system » entity system
Assigned: tedbow » Xano
Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.13 KB

What about this? It changes the delete button to a link and lets forms provide a route where the actual delete action takes place. It includes the conversion of NodeFormController as an example.

Status: Needs review » Needs work

The last submitted patch, 119: drupal_216064_119.patch, failed testing.

sun’s picture

Definitely the right direction, thanks!

  1. +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    @@ -230,30 +229,38 @@ protected function actionsElement(array $form, array &$form_state) {
    +      '#type' => 'submit',
    

    Can we move the #type to the top of the element definition, as usual?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    @@ -230,30 +229,38 @@ protected function actionsElement(array $form, array &$form_state) {
    +      $actions['delete'] = array(
    ...
    +        '#type' => 'link',
    

    I thought we had a #type button in D8 already?

    Alternatively, #type link works too, but then we need to manually apply a .button class + possibly also .button--danger class.

    At least I understood the term "link" in the proposal here to just mean the HTML element, not its presentation; i.e., even if the button is a link, it should still appear as a button. For that, we already have generic .button classes in D8 that also work for links.

    That said, given that this use-case most likely appears in many forms, I wonder whether we shouldn't introduce a dedicated #type link_button for that, so you (1) do not have to manually futz with classes and so you can still use the #button_type property that you already know from #type submit/button. That would be good for DX, I think.

Xano’s picture

Status: Needs work » Needs review
FileSize
4.24 KB

Re-roll, plus addressed points 1 and 2.

Status: Needs review » Needs work

The last submitted patch, 122: drupal_216064_122.patch, failed testing.

Xano’s picture

I thought we had a #type button in D8 already?

That's still just a generic HTML button, just like in D7 and before.

Xano’s picture

Status: Needs work » Needs review
FileSize
4.2 KB
8.86 KB

Status: Needs review » Needs work

The last submitted patch, 125: drupal_216064_125.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
16.97 KB
7.5 KB

Status: Needs review » Needs work

The last submitted patch, 127: drupal_216064_127.patch, failed testing.

The last submitted patch, 127: drupal_216064_127.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
18.39 KB
1.7 KB

This one should pass.

Xano’s picture

Issue tags: +ux

Adding tag for review to make sure we don't change UX.

Status: Needs review » Needs work

The last submitted patch, 130: drupal_216064_130.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
18.73 KB
1.19 KB

I fixed the link styling and a destination bug. If the tests pass, I expect we're pretty much done.

Status: Needs review » Needs work

The last submitted patch, 133: drupal_216064_133.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
18.69 KB
756 bytes
sun’s picture

Issue tags: -ux +Usability, +html5

Thanks a lot for resurrecting this issue, @Xano! :)

Very minor remark upfront: Can we always declare the #type property in elements first, please? :)

I tested this patch via simplytest.me and it (almost) worked as expected:

  1. The Delete link has the .button--danger class (good), but misses the base .button class, so the button styling is not actually applied :-)
  2. After manually adding the base .button class, the Delete link looks and works correctly in Bartik + Stark.
  3. The Seven theme does not implement styles for .button. :-/
  4. Bartik, on the other hand, does not implement styles for .button--danger. :-/

But nevermind. Fixing the CSS of those themes is a different issue and not to be solved here. As long as the base .button class is output, we're fine.

Second, I assume that removing the local tasks/tabs is left for a separate follow-up issue?

→ Can we just add the base .button class, and if possible, also move the #type properties to the top? :)

With those two adjustments, we should be able to move forward.


Mandatory off by one error: Usage of NestedArray for merging the query destination into the route options looks a bit excessive to me — let's replace that with a simple:

$route_info['options'] += array('query' => array());
$route_info['options']['query'] += array('destination' => ...);

That said, the intention of the current code in the patch appears to be to force-replace the destination, so that is even more simple:

$route_info['options']['query']['destination'] = ...;

:-)

Xano’s picture

FileSize
18.36 KB
1.77 KB
sun’s picture

FileSize
18.52 KB
5.39 KB

Helping you out a little bit :-)

  1. Reverted bogus chmod change to default.settings.php.
  2. Check isNew() first (performance).
  3. Specify element #type first.
larowlan’s picture

While we're at it, can we add 'use-ajax' class and 'data-accepts' => 'application/vnd.drupal-modal' attribute to these links so they use a modal?

Xano’s picture

Status: Needs review » Reviewed & tested by the community

Specify element #type first.

That was really out of scope. I don't care that much in this case, but please stick to the change the issue is about.

While we're at it, can we add 'use-ajax' class and 'data-accepts' => 'application/vnd.drupal-modal' attribute to these links so they use a modal?

Shall we do that in a follow-up so it will be easier to get this committed?

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Yeah, that's definitely a separate issue.

Wow, this is an oldie but a goodie! Unfortunately, does not seem to apply anymore. :(

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.52 KB

Merged 8.x (cleanly).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 142: entity.delete.142.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

142: entity.delete.142.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 142: entity.delete.142.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
18.63 KB

Re-roll because picture_mapping entities have been renamed.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc
#142 was random failures

effulgentsia’s picture

FileSize
88.4 KB

+1 to RTBC. Here's a screenshot.

bleen’s picture

FileSize
9.03 KB

I'm not a UX designer but it does make me sad in my sad place to see three different "buttons" right next to each other like this:

"That said, I *MUCH* prefer three different buttons to having to deal with the horrible annoyance of validation functions firing when deleting a node...

Xano’s picture

This issue does *not* change the design and never will. It only changes the inner workings of the button. If you want to change the design, please create a separate issue for that.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 146: drupal_216064_146.patch, failed testing.

floretan’s picture

Status: Needs work » Needs review
FileSize
18.81 KB

The patch from #146 didn't apply. Here's a re-rolled patch.

Status: Needs review » Needs work

The last submitted patch, 152: drupal_216064_152.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
18.71 KB
2.45 KB

this should be it

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Great. Back to RTBC then.

This issue does *not* change the design

Correct. I somehow missed that in HEAD it already is styled as a link despite being an HTML submit button, which is why I uploaded the screenshot in #148. Sorry for the confusion.

alexpott’s picture

154: drupal_216064_154.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestFormController.php
@@ -108,6 +108,21 @@ public function save(array $form, array &$form_state) {
+  protected function actions(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+    $actions['delete'] = array(
+      '#type' => 'submit',
+      '#value' => $this->t('Delete'),
+      '#submit' => array(
+        array($this, 'delete'),
+      ),
+    );
+    return $actions;
+  }

I thought we are making delete's links?

Xano’s picture

FileSize
27.21 KB
8.24 KB
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

yes true, the test entity should follow the rest of core:)

  • Commit 750a1d8 on 8.x by alexpott:
    Issue #216064 by Xano, idflood, drifter, Artusamak, ygerasimov, sun,...
alexpott’s picture

Title: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link » Change record: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Missing change record

Committed 750a1d8 and pushed to 8.x. Thanks!

On reflection I think we need a change record here as anyone adding a custom entity should be aware.

sun’s picture

Awesome! :-)

Shameless plug:

#2243575: Add #button_type support for #type link

…ensures that this special notion of "link buttons" can be properly identified/targeted by themes.

alexpott’s picture

Priority: Major » Critical
Issue tags: +beta blocker

Ugh... now this is a critical beta-blocker.

alexpott’s picture

Category: Bug report » Task
ParisLiakos’s picture

Title: Change record: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link » Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Missing change record, -beta blocker

Yay! finally :)
Change notice here https://drupal.org/node/2250341

ParisLiakos’s picture

Hm, should i move it to D7? is it back portable? (i believe the d7 patch will be a lot bigger)

xjm’s picture

Thanks for the change record!

dcam’s picture

Version: 8.x-dev » 7.x-dev

I hope this is back-portable. This is a big "WTF?" for some D7 users I know.

webchick’s picture

Status: Fixed » Patch (to be ported)
sun’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Fixed
Issue tags: -Needs backport to D6, -Needs backport to D7

Sorry, I don't see how this is back-portable in any possible way.

Doing so is guaranteed to break every existing hook_form_alter() implementation for all core "entity" forms.

In addition, there is no standardized .button CSS class for links in D7 and below, which is a major accomplishment in D8 and a major requirement for this change.

In addition, a backport would only affect forms of core entity types; the forms of contributed + custom entity types would get completely out of sync.

In short, this can be back-ported, but has to be an explicit opt-in, so developers + themers are aware that they have to adjust. → Contrib module.

Status: Fixed » Closed (fixed)

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