Hi,

here is a typical scenario:

(1) create article node/1
(2) add required field A to node type Article.
(3) go to node/1 and click delete
(4) system will require you to fill in field A before asking you to confirm the deletion.

Since we are deleting the node anyway, it shouldn't be important to validate the fields, no?

Cheers,

Albert.

CommentFileSizeAuthor
#15 1342444-node-validation-on-delete-pass.patch2.69 KBagentrickard
#13 1342444-node-validation-on-delete-pass.patch2.55 KBagentrickard
#7 1342444-node-validation-on-delete-pass.patch0 bytesagentrickard
#4 1342444-node-validation-on-delete-pass.patch2.57 KBagentrickard
#3 1342444-node-validation-on-delete-fail.patch1.54 KBagentrickard
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Absolutely. Let's get a patch going.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Great. Now that we have client-side form validation, this becomes much harder to test.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Active » Needs review
FileSize
1342444-node-validation-on-delete-fail.patch1.54 KB

Patch with a failing test. Note that I had to add a new test for the delete confirm behavior.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
FileSize
1342444-node-validation-on-delete-pass.patch2.57 KB

And a patch that should pass.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Category: feature » task
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

First patch failed as expected.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
FileSize
1342444-node-validation-on-delete-pass.patch0 bytes

Fixed a comment in the pass version of the test.

alberto56’s picture

alberto56 CreditAttribution: alberto56 commented

@agentrickard your patch in comment #7 is said to contain 0 bytes. Should we review the one in comment #4 or wait for a new version?

Cheers,

Albert.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs review » Needs work

Oops. Review the version in #4, which contains a comment "this should fail." that needs to be edited.

Robin Millette’s picture

Robin Millette CreditAttribution: Robin Millette commented

Once it's approved for D8, will this get backported or is it considered an API change?

joachim’s picture

joachim CreditAttribution: joachim commented
Issue tags: +Needs backport to D7

I say it should be backported. It makes deleting anything with required fields a total pain for users.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Not an API change. THe patch just needs a minor re-roll.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs work » Needs review
FileSize
1342444-node-validation-on-delete-pass.patch2.55 KB

Finally got around to a re-roll.

Status: Needs review » Needs work

The last submitted patch, 1342444-node-validation-on-delete-pass.patch, failed testing.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs work » Needs review
FileSize
1342444-node-validation-on-delete-pass.patch2.69 KB

And a re-re-roll. Should be ready to go.

dcam’s picture

dcam CreditAttribution: dcam commented

I tested #15 and it fixes the issue for me.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs review » Reviewed & tested by the community
agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Reviewed & tested by the community » Needs review

With the new Entity system, do we actually need to back this logic up a level to EntityFormController.php?

dcam’s picture

dcam CreditAttribution: dcam commented
Title: Do not validate fields if node is in the process of being deleted » Do not validate fields if entity is in the process of being deleted
Component: node system » entity system
Status: Needs review » Needs work

Yeah, this does need to be applied to EntityFormController. Perform the same test described in the OP, except with a taxonomy vocabulary and term. If you try to delete a term with a required, empty field this form also forces you to fill out the field before deleting.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented

Makes sense.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

Can this be merged into #216064: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link?

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
CreditAttribution: agentrickard commented
Status: Needs work » Closed (duplicate)

Yes.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

Awesome, thanks. I just linked here from the other issue.