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.
Comment | File | Size | Author |
---|---|---|---|
#158 | interdiff.txt | 8.24 KB | Xano |
#158 | drupal_216064_158.patch | 27.21 KB | Xano |
#146 | drupal_216064_146.patch | 18.63 KB | Xano |
#85 | delete_button_to_link-216064-85.patch | 6.47 KB | tedbow |
#82 | delete_button_to_link-216064-82.patch | 4.2 KB | tedbow |
Comments
Comment #1
decafdennis CreditAttribution: decafdennis commentedSince 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: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 letdrupal_process_form
calldrupal_redirect_form
before evendrupal_validate_form
is called.Comment #2
catchI can see how this'd be really, really annoying.
Rolled the above into a patch, untested.
Comment #3
GreenLED CreditAttribution: GreenLED commentedRan 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...
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.
Confirmation:
1. Reversed patch, above error disappeared.
» Respectfully, GreenLED
»
Comment #4
GreenLED CreditAttribution: GreenLED commentedChanging status.
» Respectfully, GreenLED
»
Comment #5
decafdennis CreditAttribution: decafdennis commentedQuick 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.
Comment #6
perforator CreditAttribution: perforator commentedHi.
Is there any progress on this issue? In Drupal 6.3 it's obviously not fixed.
Comment #7
AltaVida CreditAttribution: AltaVida commentedI 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?).
Comment #8
nextdude CreditAttribution: nextdude commentedThis issue is still happening in 6.9. Is anyone still working on this?
Comment #9
coltraneWork is happening on this to an effect for 7 at #370823: Allow Nodes to Be Previewed Without Required Fields.
Comment #10
kenorb CreditAttribution: kenorb commentedGot the same problem.
Comment #11
Pasqualleit 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)
Comment #12
Aren Cambre CreditAttribution: Aren Cambre commented#300921: Node Deletion Validates Required Fields marked as duplicate of this.
Comment #13
sunsubscribing
Comment #14
gpk CreditAttribution: gpk commented#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!!!
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedYes. 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.Comment #16
gpk CreditAttribution: gpk commentedMarked #545206: Cannot delete node if CCK required field is empty as duplicate.
Comment #17
thekevinday CreditAttribution: thekevinday commentedsubscribing
Comment #18
matt2000 CreditAttribution: matt2000 commentedThis has often annoyed me too.
Comment #19
raimondious CreditAttribution: raimondious commentedThis 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.
Comment #20
mitchmac CreditAttribution: mitchmac commentedsubscribing
Comment #21
StevenPatzIs 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.
Comment #22
ryumkin CreditAttribution: ryumkin commentedThis 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.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedFixes 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).
Comment #24
ygerasimov CreditAttribution: ygerasimov commentedI 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.
Comment #25
ygerasimov CreditAttribution: ygerasimov commentedComment #26
chx CreditAttribution: chx commentedThis 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.
Comment #27
ygerasimov CreditAttribution: ygerasimov commentedAlright. Lets concentrate on first four forms from #24 first. I will write tests and put comments.
Comment #28
chx CreditAttribution: chx commentedOh, by the way why we can't fix those three? if there is no #type value to hang your hat on, well, add one.
Comment #29
ygerasimov CreditAttribution: ygerasimov commentedI 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.
Comment #30
ygerasimov CreditAttribution: ygerasimov commented#29: delete_menu.patch queued for re-testing.
Comment #31
idflood CreditAttribution: idflood commentedUser 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.
Comment #32
idflood CreditAttribution: idflood commentedI 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.
Comment #33
idflood CreditAttribution: idflood commentedComment #34
idflood CreditAttribution: idflood commentedLet'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.
Comment #35
idflood CreditAttribution: idflood commentedsame as before but without broken last line in patch file...
Comment #37
sun@idflood: Please merge those patches into a single one. (just like the one in #32)
Comment #38
idflood CreditAttribution: idflood commented@sun: here it is. I didn't looked at those failed tests yet.
Comment #39
idflood CreditAttribution: idflood commentedThe fix was not in the right place for user.admin.inc. This sould fix at least 1 test.
Comment #41
DamienMcKennaJust ran into this.
Comment #42
marcingy CreditAttribution: marcingy commentedGrabing I had a more exacting patch for nodes will work on this over the next couple of days
Comment #43
marcingy CreditAttribution: marcingy commentedLooks good
Comment #44
catchLooks like a mis-post.
Comment #45
marcingy CreditAttribution: marcingy commentedThere was a typo in the patch - reroll.
Comment #46
attiks CreditAttribution: attiks commentedsubscribing for #1239910: META: tracking other issues about validation
Comment #47
ArtusamakYay, 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.
Comment #48
marcingy CreditAttribution: marcingy commentedAdding backport tag
Comment #49
higherform CreditAttribution: higherform commentedI 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!
Comment #50
marcingy CreditAttribution: marcingy commentedDrupal 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.
Comment #51
marcingy CreditAttribution: marcingy commentedIt has tests now so removing that tag.
Comment #52
ArtusamakThe patch in #45 is also appliable to D7. Let's attach the patch for review.
Comment #53
ArtusamakSorry, rerolling with 7.x
Comment #54
marcingy CreditAttribution: marcingy commentedPlease leave at d8 as it needs to be fixed there first
Comment #55
ArtusamakThe 7.x-dev version modification was to tell the bot to roll the patch for D7.
Comment #56
pjcdawkins CreditAttribution: pjcdawkins commentedSubscribe
Comment #57
sun1) 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.
Missing phpDoc.
20 days to next Drupal core point release.
Comment #58
pwaterz CreditAttribution: pwaterz commentedThis is also a problem on #ajax, I can't seem to stop the button from validating the form on the ajax callback.
Comment #59
xgougeon CreditAttribution: xgougeon commentedRunning 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 !
Comment #60
pjcdawkins CreditAttribution: pjcdawkins commentedHi, @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.
Comment #61
xgougeon CreditAttribution: xgougeon commentedHi pjcdawkins. Thanks for your help, and sorry about that I was dealing with several problems at once and got confused...
Comment #62
ArtusamakLet's try that again!
Comment #63
attiks CreditAttribution: attiks commentedNow there's a mix of using t()
one string with and the other without t()
dito
dito
dito
dito
dito
dito
Comment #64
ArtusamakReposting with the appropriate fix.
Comment #65
ArtusamakFeedback 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. :)
Comment #66
drifter CreditAttribution: drifter commentedRerolling, updated tests for new locations, and the node form to EntityFormController. As a bonus, we'll get this fix for all entities using EntityFormController.
Comment #68
drifter CreditAttribution: drifter commentedA better way to determine the entity type in the EntityFormController, plus fixes for role deletion.
Comment #70
star-szr@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.
Comment #71
drifter CreditAttribution: drifter commentedOK, let's try that... the failing tests passed for me locally.
Comment #73
drifter CreditAttribution: drifter commentedHmm, didn't do my merges correctly. Sorry about that.
Comment #74
star-szrThanks @drifter! To move this even further along, there are a few minor code conventions that can be cleaned up:
Trailing whitespace here.
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.
Comment #75
star-szr#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).
Comment #76
drifter CreditAttribution: drifter commentedThanks for the review. Fixing up the comments and whitespace.
Comment #77
tedbow@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
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'
Comment #78
agentrickardEasier solution, of course, is to move "delete" to a separate callback / page tab. Maybe turn the button into a link.
Comment #79
tedbowA 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?
Comment #80
fuzzy76 CreditAttribution: fuzzy76 commentedDelete 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...
Comment #81
jcisio CreditAttribution: jcisio commented@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.
Comment #82
tedbowOk 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
Comment #84
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.
Comment #85
tedbowOk, I have added some changes to tests. Hopefully this will pass all.
Changing title to reflect the fix
Comment #86
tedbowOk so it passed all the tests. Any reviews would be good.
I think it should be ok as a link because:
Hopefully we can get this in before feature freeze.
Comment #87
agentrickardI 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.
Comment #88
sunI think this should be postponed on #1834002: Configuration delete operations are all over the place
Comment #89
agentrickardI'd rather not delay a long-delayed older issue in favor of a newer problem.
Comment #90
tedbowI 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.
Comment #91
gpk CreditAttribution: gpk commentedAgree, 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.
Comment #92
tedbowI 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.
Comment #93
agentrickardCan we get one more review on the merits of the code in this patch and then RTBC, please.
Comment #94
attiks CreditAttribution: attiks commentedis this always going to be '/delete'?
Comment #95
tedbowI 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.
Comment #96
agentrickardIt should always be 'delete'. The issue that @sun links to in #88 should be a follow-up to enforce that standard.
Comment #97
star-szrMinor docs cleanup and a couple indentation and whitespace fixes.
Comment #98
tedbow@Cottser
Thanks for that!
Comment #99
tedbowWould 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?
Comment #100
idflood CreditAttribution: idflood commentedReroll of patch in #97 but removed the @todo since #96 seems to confirm it is correct.
Comment #102
idflood CreditAttribution: idflood commented#100: delete_button_to_link-216064-100.patch queued for re-testing.
Comment #104
star-szr#100: delete_button_to_link-216064-100.patch queued for re-testing.
Comment #105
jibran#100: delete_button_to_link-216064-100.patch queued for re-testing.
Comment #106
star-szr#100: delete_button_to_link-216064-100.patch queued for re-testing.
Comment #108
star-szrTagging for reroll.
Comment #109
TravisCarden CreditAttribution: TravisCarden commentedComment #110
deastlack CreditAttribution: deastlack commentedsubscribe
Comment #111
TravisCarden CreditAttribution: TravisCarden commentedFYI, @deastlack: Stop subscribing, start following. :)
Comment #112
tedbow@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)
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?
Comment #113
tedbow1 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?
Comment #114
tedbowOk, here is patch that implements by solution 2 from #112.
It takes away reliance on $entity->uri() and adds
This then determines in EntityFormController->actions() whether to create a link or a button.
Comment #116
tedbowSo 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
Comment #117
sunClarifying the issue title.
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().
Comment #118
dcam CreditAttribution: dcam commentedMarked #1970342: Limit validation errors for the entity form delete button as a duplicate.
Comment #118.0
dcam CreditAttribution: dcam commentedadd summary
Comment #119
XanoWhat 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.Comment #121
sunDefinitely the right direction, thanks!
Can we move the #type to the top of the element definition, as usual?
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.
Comment #122
XanoRe-roll, plus addressed points 1 and 2.
Comment #124
XanoThat's still just a generic HTML button, just like in D7 and before.
Comment #125
XanoComment #127
XanoComment #130
XanoThis one should pass.
Comment #131
XanoAdding tag for review to make sure we don't change UX.
Comment #133
XanoI fixed the link styling and a destination bug. If the tests pass, I expect we're pretty much done.
Comment #135
XanoComment #136
sunThanks 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:
.button--danger
class (good), but misses the base.button
class, so the button styling is not actually applied :-).button
class, the Delete link looks and works correctly in Bartik + Stark..button
. :-/.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:
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:
:-)
Comment #137
XanoComment #138
sunHelping you out a little bit :-)
Comment #139
larowlanWhile 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?
Comment #140
XanoThat was really out of scope. I don't care that much in this case, but please stick to the change the issue is about.
Shall we do that in a follow-up so it will be easier to get this committed?
Comment #141
webchickYeah, that's definitely a separate issue.
Wow, this is an oldie but a goodie! Unfortunately, does not seem to apply anymore. :(
Comment #142
sunMerged 8.x (cleanly).
Comment #144
ParisLiakos CreditAttribution: ParisLiakos commented142: entity.delete.142.patch queued for re-testing.
Comment #146
XanoRe-roll because picture_mapping entities have been renamed.
Comment #147
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
#142 was random failures
Comment #148
effulgentsia CreditAttribution: effulgentsia commented+1 to RTBC. Here's a screenshot.
Comment #149
bleen CreditAttribution: bleen commentedI'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...
Comment #150
XanoThis 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.
Comment #152
floretan CreditAttribution: floretan commentedThe patch from #146 didn't apply. Here's a re-rolled patch.
Comment #154
ParisLiakos CreditAttribution: ParisLiakos commentedthis should be it
Comment #155
effulgentsia CreditAttribution: effulgentsia commentedGreat. Back to RTBC then.
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.
Comment #156
alexpott154: drupal_216064_154.patch queued for re-testing.
Comment #157
alexpottI thought we are making delete's links?
Comment #158
XanoComment #159
ParisLiakos CreditAttribution: ParisLiakos commentedyes true, the test entity should follow the rest of core:)
Comment #161
alexpottCommitted 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.
Comment #162
sunAwesome! :-)
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.
Comment #163
alexpottUgh... now this is a critical beta-blocker.
Comment #164
alexpottComment #165
ParisLiakos CreditAttribution: ParisLiakos commentedYay! finally :)
Change notice here https://drupal.org/node/2250341
Comment #166
ParisLiakos CreditAttribution: ParisLiakos commentedHm, should i move it to D7? is it back portable? (i believe the d7 patch will be a lot bigger)
Comment #167
BerdirNoticed #2250377: ProfileForm::actions() defines new delete element instead of altering it as it expects because of #2084323: EntityForm::actions() adds 'delete' without checking access :)
Comment #168
xjmThanks for the change record!
Comment #169
dcam CreditAttribution: dcam commentedI hope this is back-portable. This is a big "WTF?" for some D7 users I know.
Comment #170
webchickComment #171
sunSorry, 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.