Problem/Motivation

Most bundle entities don't allow deletion if there are entities of that bundle. For example, you can't delete the 'article' node type if there are 'article' nodes.

The following form classes which enforce this logic are almost identical:

- BlockContentTypeDeleteForm
- MediaTypeDeleteConfirmForm
- NodeTypeDeleteConfirm

All these bundle entity delete forms are reinventing the wheel, and contrib bundle entity types are having to do this too.

(Note that vocabulary entities are different -- deleting a vocabulary causes the deletion of all the terms.)

Steps to reproduce

Proposed resolution

Provide a generic BundleEntityDeleteForm class. In most cases, this could be used as-is by bundle entity types.

Given that we have CommentTypeDeleteForm which has additional checks for allowing deletion, I suggest we have:

- buildForm() is generic and reusable
- the actual logic goes in checkMayDelete(), which returns an array of reasons for disallowing deletion. This is what CommentTypeDeleteForm could override.

Remaining tasks

Needs change record review, of both change records.

User interface changes

Before with JS

Before no JS

After with JS

After no JS

Introduced terminology

API changes

A new BundleEntityDeleteForm is provided for entity types using bundle entities.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#46 Before-nojs.png24.47 KBquietone
#46 After-nojs.png23.06 KBquietone
#43 After.png23.21 KBquietone
#41 After.png23.34 KBquietone
#41 Before.png20.66 KBquietone

Issue fork drupal-3571580

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.

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

joachim’s picture

joachim commented

@ben280398 I'd started working on this too BTW.

ben280398’s picture

ben280398 commented

Got it @joachim. Don't want to step on any toes here so I'll work on something else man.

ben280398’s picture

ben280398 commented

Got it @joachim. Don't want to step on any toes here so I'll work on something else instead man

ben280398’s picture

ben280398 commented

Don't want to step on any toes here @joachim, so I can work on something else, no worries man :) (also my previous comments seem to have disappeared so this is attempt #3)

joachim opened merge request !14651

joachim’s picture

joachim commented
Status: Active » Needs work

This is the sort of approach I was thinking.

Still to convert: content block and media.

joachim’s picture

joachim commented

We can remove some tests too -- things like this 

    // Attempt to delete the content type, which should now be allowed.
    $this->drupalGet('admin/structure/types/manage/' . $type->label() . '/delete');
    $this->assertSession()->pageTextContains("Are you sure you want to delete the content type {$type->label()}?");
    $this->assertSession()->pageTextContains('This action cannot be undone.');

in NodeTypeTest and maybe elsewhere should be moved to one of the test entity types.

joachim’s picture

joachim commented

The following tests can be replaced by a generic test:

testBlockContentTypeDeletion
MediaUiJavascriptTest
NodeTypeTest.php

joachim’s picture

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

joachim commented
Issue summary: View changes
Status: Needs review » Active
joachim’s picture

joachim commented
Status: Active » Needs review
joachim’s picture

joachim commented
Issue summary: View changes
joachim’s picture

joachim commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think if we're going to generalize this, then it would be useful to allow to delete the content at this point. See \Drupal\paragraphs\Form\ParagraphsTypeDeleteConfirm for example, for paragraphs, this is more important than others, because there's no UI for them, and if there are for some reason some leftovers then it can be hard to delete them.

It would need to be done in a way that scales, so a batch or so, paragraphs doesn't do that yet.

joachim’s picture

joachim commented

What Paragraphs has is this:

      // Optional to delete existing entities.
      $form['delete_entities'] = [
        '#type' => 'submit',
        '#submit' => [[$this, 'deleteExistingEntities']],
        '#value' => $this->formatPlural($num_paragraphs, 'Delete existing Paragraph', 'Delete all @count existing Paragraphs'),
      ];

Adding this to core is increasing the scope of this issue by quite a bit! But I can see that the current way I've made the form wouldn't leave space for this.

Not sure how best to proceed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Wouldn't we want two separate form classes here, so you can opt in to either functionality? Perhaps we add the "delete entities" button in a followup, although we likely want to take it into account when naming the class here I think.

joachim’s picture

joachim commented

I don't yet see how the delete entities thing would work with the getReasons method I've added.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Maybe keep a "reasons" method for additional reasons, but have a separate method for counting bundleOf entities? Only allow the delete if the count is non-zero but there are no additional reasons?

joachim’s picture

joachim commented

> Maybe keep a "reasons" method for additional reasons, but have a separate method for counting bundleOf entities?

Yeah that's what I'm thinking so far.

> I think if we're going to generalize this, then it would be useful to allow to delete the content at this point

We already have 'delete-multiple-form' link templates and an DeleteMultipleForm. Should this be expanded to allow a bundle route parameter? But it's really extending the scope.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Maybe you can link off to the delete-multiple-form for the bundleOf entity, and set destination back to the parent entity delete form? That makes it two explicit steps instead of one.

edit: oh, but we're only deleting a bundle, not the entire entity type, so that needs extending too :/

joachim’s picture

joachim commented

> edit: oh, but we're only deleting a bundle, not the entire entity type, so that needs extending too :/

Yeah.... I think that extending the entity delete system should be a follow-up, which we can leave space for here.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Need some opinions, as I can see two ways to do this:

A. We add a button 'Delete all content entities', and handle the submission of that in BundleEntityDeleteForm.

B. We make part of the 'You may not remove this @bundle-label until you have removed all of the %type @plural-content-label.' text a link to the content entity type's 'delete-multiple-form' link template. Adding the link would be done as a follow-up, as Drupal\Core\Entity\Form\DeleteMultipleForm does not yet support bundles.

I sort of prefer B, as adding a separate place where deletion is done feels messy.

joachim’s picture

joachim commented
Status: Needs work » Needs review

Ready for review.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Status: Needs review » Needs work

The Pipeline has failed with following error,

FILE: /builds/core/lib/Drupal/Core/Entity/BundleEntityDeleteForm.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
75 | ERROR | [x] Expected "int" but found "integer" for function return type
| | (Drupal.Commenting.FunctionComment.InvalidReturn)

joachim’s picture

joachim commented
Status: Needs work » Needs review

Fixed.

smustgrave’s picture

smustgrave commented

Appears to be 3 open threads if those could be answered.

This is looking really awesome btw!

joachim’s picture

joachim commented

Realised that we need to keep the Cancel button, for users without JS, as they will see this form on its own page rather than a modal.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Did a rebase for the random failures and it's all green

But do agree with the open threads can we add the test coverage back?

joachim’s picture

joachim commented
Status: Needs work » Needs review

Restored the entity-specific tests.

But I really don't see the point -- we have a test with a generic entity type. So what is the point of testBlockContentTypeDeletion() -- it's a whole functional test, a significant amount of resources, which just repeats what BundleDeleteFormTest now does for the generic case. What is this for?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe that was the only feedback LGTM

acbramley’s picture

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

Tests are failing, also think we should add a couple more lines to the entity_test coverage.

joachim’s picture

joachim commented
Status: Needs work » Needs review

Fixed the tests failures.

smustgrave’s picture

smustgrave commented

Changes look good to me, before I mark though @Joachim did you want to convert to a kernel? I don't have an opinion.

joachim’s picture

joachim commented

Could do -- is @acbramley happy with the new test not submitting the form?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks @joachim and @acbramley for following up in this on slack. Feedback appears to be addressed to me. LGTM

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

One question on the MR.

joachim’s picture

joachim commented

Done.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback from catch addressed

quietone’s picture

quietone commented
Issue summary: View changes
Issue tags: +Usability
StatusFileSize
new Before.png20.66 KB
new After.png23.34 KB

I read the comments and the change record. I didn't find any unanswered questions and I updated credit. What I didn't find is a review of the change record..

And this is adding a form so should be tagged, usability, per the Usability Core Gate. I am adding the tag now. And as the UI is being changed there should be a screen shot of the change or changes.

I applied the diff and did limited testing but enough to have a before and after screenshot. In the after screenshot I was surprised to see the period at the end of the title as other similar forms do not include a period. Only a '?' if the title is a question. The new form also has two ways to cancel, one at the upper right and the Cancel button.

Back to the change record. The change record is written for developers. But this change also affect site builders and admins so it should include a sample screen shot etc for those users.

joachim’s picture

joachim commented

> In the after screenshot I was surprised to see the period at the end of the title as other similar forms do not include a period. Only a '?' if the title is a question.

Good point. I can remove it but I can't do a new screenshot on my current system.

EDIT: full stop removed.

quietone’s picture

quietone commented
Issue summary: View changes
StatusFileSize
new After.png23.21 KB

Thanks for the prompt wee change. I uploaded a new after screenshot. I do think it looks better.

joachim’s picture

joachim commented

> The new form also has two ways to cancel, one at the upper right and the Cancel button.

The reason for this is that if you don't have JS, instead of a pop-up, you are taken to the form on a page of its own. In that case, there is no modal title bar with an [x]. Without a cancel button, the only way back would be the browser Back button, which is not good UX.

catch’s picture

catch
he/him
Primary language English
commented

But this change also affect site builders and admins so it should include a sample screen shot etc for those users.

It might be worth a separate change record for the UX change? Something like 'Form structure of bundle entity delete forms has changed' with the screenshots. Have only done a quick re-review of the code but overall it looks encouraging.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record review
StatusFileSize
new After-nojs.png23.06 KB
new Before-nojs.png24.47 KB

I have added a new CR based on the last comment and added screen shots in it. That will need a review.

I also updated the existing CR to change the tense to be the present tense, as in this is what it does, not what is should do.

quietone’s picture

quietone commented

Actually, maybe needs work to move the cancel button to the right in the nojs version?

joachim’s picture

joachim commented

The form structure CR looks fine.

> Entities which are bundles of another entity type will not be deleted if there are entities of that bundle

This is incorrect. They *should not*. If you don't guard against this, your form CAN delete them and then your site is broken.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record review

CR looks fine to me also, believe that was the last think?

Sorry @joachim fell off my radar

  • catch committed 5dcd446c on 11.x 
    task: #3571580 add a generic bundle type entity delete form

By: joachim...

  • catch committed 1753b953 on main 
    task: #3571580 add a generic bundle type entity delete form

By: joachim...
catch’s picture

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

Sorry this took a while to get back to. Committed/pushed to main and 11.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.

godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Needs work

This broke an MR pipeline against 11.x: https://git.drupalcode.org/project/drupal/-/jobs/12917008
core/tests/Drupal/KernelTests/Core/Entity/BundleDeleteFormTest.php has the wrong namespace: Drupal\FunctionalTests\Entity\BundleDeleteFormTest. It should be Drupal\KernelTests\Core\Entity\BundleDeleteFormTest.

We probably also need to look into why that isn't being detected on main.

godotislate opened merge request !17465

godotislate opened merge request !17467

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

MR with fix: https://git.drupalcode.org/project/drupal/-/merge_requests/17465
MR against 11.x to show tests pass: https://git.drupalcode.org/project/drupal/-/merge_requests/17467

  • catch committed 756d1fc9 on 11.x 
    task: #3571580 add a generic bundle type entity delete form

By: joachim...

  • catch committed a7c1655a on main 
    task: #3571580 add a generic bundle type entity delete form

By: joachim...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Needs review » Fixed

Committed/pushed the follow-up to main and 11.x - we have an issue open about the failure to detect namespace mis-matches, although can't find it right now.

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.

godotislate’s picture

godotislate
he/him
commented

@catch were you thinking of #3612498: PhpUnitApiFindAllClassFilesTest not finding namespace errors? That one got in last month, but it looks like doesn't cover this case.

catch’s picture

catch
he/him
Primary language English
commented

@godotislate - yes that one, and that's annoying.

godotislate’s picture

godotislate
he/him
commented

Opened #3631345: PhpUnitApiFindAllClassFilesTest still not finding all namespace errors.

Status: Fixed » Closed (fixed)

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