Problem/Motivation

When deleting a crop type, any image styles that make use of it are also deleted.

However, the crop type delete form doesn't warn the user of this.

This is because CropTypeDeleteForm extends EntityConfirmFormBase. It should use EntityDeleteForm instead, which makes use of addDependencyListsToForm().

Setting as critical, since this is data loss.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#4 3240807-4-CropTypeDeleteForm-extends-EntityDeleteForm.patch640 bytesdrdam
#3 3240807-3-CropTypeDeleteForm-extends-EntityDeleteForm.patch640 bytesdrdam
#2 3240807-2-CropTypeDeleteForm-extends-EntityDeleteForm.patch380 bytesdrdam

Issue fork crop-3240807

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

joachim created an issue. See original summary.

drdam’s picture

drdam commented
Status: Active » Needs review
StatusFileSize
new 3240807-2-CropTypeDeleteForm-extends-EntityDeleteForm.patch380 bytes
drdam’s picture

drdam commented
StatusFileSize
new 3240807-3-CropTypeDeleteForm-extends-EntityDeleteForm.patch640 bytes
drdam’s picture

drdam commented
StatusFileSize
new 3240807-4-CropTypeDeleteForm-extends-EntityDeleteForm.patch640 bytes

drdam opened merge request !16

drdam’s picture

drdam commented
Status: Needs review » Reviewed & tested by the community
nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs change record

Thanks @drdam for your MR.
Changes looks good, and it makes perfect sense to implement it.
And it looks like change record required to deploy these changes.

Can you please update delete functional test.
Probably this can be implemented at CropFunctionalTest::testCropTypeCrud().

drdam’s picture

drdam commented
Status: Needs work » Needs review
drdam’s picture

drdam commented
Status: Needs review » Reviewed & tested by the community
nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Reviewed & tested by the community » Needs work

@drdam please provide change record to this issue.
And test doesn't seem to be updated.
Please check my feedback.
Set back to NW.

drdam’s picture

drdam commented
Status: Needs work » Needs review

@nikolay shapovalov : sorry for that, after nearly 2 month, I have presume it be ok for you ...

phenaproxima made their first commit to this issue’s fork.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs change record

I don't think this needs a change record. Forms are internal. I think this fix makes a ton of sense, and I confirmed that the test (with a few changes) passes when merged with 8.x-2.x.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Fixed

Merge train started for 8.x-2.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • phenaproxima committed ad156ac2 on 8.x-2.x authored by drdam 
    fix: #3240807 Crop type delete form doesn't warn of deleting dependent...

Status: Fixed » Closed (fixed)

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