Problem/Motivation

WorkflowDeleteForm extends EntityConfirmFormBase, not EntityDeleteForm. EntityDeleteForm displays consequences of deleting configuration dependencies. Currently a workflow config entity can be deleted without the site admin being aware of consequential configuration changes.

Steps to reproduce

1. Delete a workflow
2. See confirmation page without config changes

Proposed resolution

Change base class

Remaining tasks

Do it!

User interface changes

Deleting a workflow displays "Configuration updates", if any.

API changes

None

Data model changes

None

Release notes snippet

Comments

mglaman created an issue. See original summary.

mglaman’s picture

Issue tags: +Novice
sidharth_soman’s picture

StatusFileSize
new673 bytes

I have changed the base class. Please review.

longwave’s picture

Status: Active » Needs work

Thanks - @sidharth_soman please note there is no need to run tests on all available databases, as each test costs the Drupal Association money to run, and it is usually only necessary to run tests once with the default setting by setting the issue to "Needs review".

I think that some of the methods in WorkflowDeleteForm can now also be cleaned up here, as EntityDeleteFormTrait provides them already.

sidharth_soman’s picture

@longwave - I wasn't the one who applied all those tests. I had simply uploaded the patch without adding any tests. Not sure how that happened.

longwave’s picture

Apologies, I assumed and did not check.

@rushiraval - please see #4 as it appears you added multiple tests here. In most cases just set the status to "Needs review" and a relevant test will be run automatically.

roshni27’s picture

Status: Needs work » Needs review
StatusFileSize
new1.55 KB
new1.01 KB

Hello,
I removed the getQuestion(), getCancelUrl(), and getConfirmText() methods because these methods are already provided by the EntityDeleteFormTrait. There's no need to redefine them unless you want to customize their behavior.
The submitForm method has also been removed because EntityDeleteFormTrait handles entity deletion, message display, and redirection automatically.

Please review the patch.

longwave’s picture

Status: Needs review » Active

  7 | WARNING | [x] Unused use statement
    |         |     (Drupal.Classes.UnusedUseStatement.UnusedUse)
 27 | ERROR   | [x] Expected 1 blank line after function; 0 found
    |         |     (Squiz.WhiteSpace.FunctionSpacing.AfterLast)
 28 | ERROR   | [x] The closing brace for the class must have an
    |         |     empty line before it
    |         |     (Drupal.Classes.ClassDeclaration.CloseBraceAfterBody)
roshni27’s picture

Status: Active » Needs review
StatusFileSize
new1.58 KB

Status: Needs review » Needs work

The last submitted patch, 9: 3377166-7.patch, failed testing. View results

roshni27’s picture

Status: Needs work » Needs review
StatusFileSize
new1.36 KB

Please review.

smustgrave’s picture

Status: Needs review » Needs work

Also interdiffs should be included.

roshni27’s picture

Status: Needs work » Needs review
StatusFileSize
new1.08 KB
new536 bytes

Please review.

roshni27’s picture

roshni27’s picture

roshni27’s picture

StatusFileSize
new1.12 KB
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative, +Bug Smash Initiative

Applied patch and confirmed I can still delete workflows without issue

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

The issue summary pointed out one of the primary reasons this was a problem

Currently a workflow config entity can be deleted without the site admin being aware of consequential configuration changes.

Had there been test coverage to check for this information, the problem would have been caught/addressed much sooner (or never been an issue at all). Lets add tests that validate this fix and prevent it from regressing.