Allow removing a module's content entities prior to module uninstallation

+++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
@@ -0,0 +1,89 @@
+      '#title' => $this->t('Prepare uninstall'),
+      '#description' => $this->t('Clicking on this button, all module content entities data will be removed.'),
+    );

This sentence does not make sense. Also, we really need a confirmation form for this.

That form *is* the confirmation form, we don't need another.

This is right version to run tests
@tduong about #19 see https://www.drupal.org/node/2648064

1. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -136,6 +138,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
          if (isset($validation_reasons[$module_key])) {
   +        $link = Link::fromTextAndUrl(t('here'), Url::fromRoute('system.prepare_modules_entity_uninstall', ['entity_type_id' => $module_key]));
   +        $validation_reasons[$module_key][] = $this->t('To delete these content data go <a>' . $link->toString() . '</a>');
   

   entity type and module is no the same thing. You can't add this here, you have to add it in ContentUninstallValidator

2. +++ b/core/modules/system/src/Tests/Module/PrepareUninstallTest.php
   @@ -0,0 +1,72 @@
   +
   +use Drupal\search_api\Tests\WebTestBase;
   +
   

   that's why the test doesn't work.

+++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
@@ -41,6 +43,9 @@ public function validate($module) {
+        $link = Link::fromTextAndUrl(t('here'), Url::fromRoute('system.prepare_modules_entity_uninstall', ['entity_type_id' => $entity_type->id()]));
+        $reasons[] = $this->t('To delete these content data go <a>' . $link->toString() . '</a>');
       }

that's not how links work :) We might also want to make this part of the same reason string, not two separate ones.

I quite like this solution, just updated some of the wording.

Forgot to update the test.

I think from a UX point of view it's better to be explicit.

The main problem with mentioning the type in messages like that is getting case and plural forms correctly. Which is practically impossible.

This is a really big improvement!

Just some UI things to tweak:

> There are 2 Content entities to remove!

I don't think exclamation mark is needed. We don't use those anywhere else in UI strings.

Though what we should have is the standard warning that 'This action cannot be undone.' And maybe that should go with some further help text, such as we have when you delete a field? Eg, 'This will delete all Foo entities from the site, and will allow the Foobar module to be uninstalled. This action cannot be undone.'

The button could also maybe be changed to say 'Delete all Foo entities'.

Lastly, there's no need for a fieldset that consists of the whole form.

Looking at this some more, this should be following the same UI pattern as other deletion forms.

I'm having a look at making this use ConfirmFormBase or ConfirmFormInterface, but getting a bit stuck with getting the entity type ID into the form...

I'm working on this at the moment.

Updated patch:

- changed form class to inherit from ConfirmFormBase, so we get:
-- standard title with the confirmation question
-- cancel button
- changed confirm button to use the entity type label for additional clarity
- fixed entity type ID rather than label used in one of the batch labels
- fixed whitespace at the end

I'm not massively keen on this in buildForm():

    $this->entityTypeId = $entity_type_id;

but the only way to avoid it that I see is adding a form builder, which seems like overkill.

EDIT: drat, this will possibly break the test due to the button label change :( And I'm out of coding time for this weekend. Sorry!

Quick update to fix tests.

Although untested due to local environment issues.

Ooops

Generally we do not use the word "entities" in the UI. This is part of our guidelines. Can we in this case not say:

"Delete all Content types"

Unless I am wrong, for core almost all "bundles" are referred to as types in the UI. I would keep that consistent pattern.

I would happily have a philosophical debate over the label, but its a established standard - if one wishes to re-open that discussion we need a separate issue, and avoid negatively impacting the importance of this issue.

> Generally we do not use the word "entities" in the UI

Fair enough.

Though "Delete all Content types" isn't what we're doing here. We're deleting all nodes / comments / menu links / flaggings.

Entity types have plural labels as of 8.1.x, so we can use these:

 *   label_plural = @Translation("content items"),
 *   label_count = @PluralTranslation(
 *     singular = "@count content item",
 *     plural = "@count content items"
 *   ),

Ahh, good point. Great!

Updating with suggestion from #55.

Looks like you uploaded the interdiff twice?

ok, this should be correct now.

+++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
@@ -0,0 +1,146 @@
+    $context['results']['entity_type_plural'] = ucfirst($entity_type->getPluralLabel());
...
+    drupal_set_message(t('@entity_type_plural have been deleted.', [
+      '@entity_type_plural' => $results['entity_type_plural'],
+    ]));

We can't rely on the replacement token being at the start of the sentence in other languages, so ucfirst() is wrong here.

Best way is to reword so the token is not at the start of the sentence.

Looks like we're in a pretty good place.

The grammar police say "allow" cannot be followed immediately by an infinitive, but gerund phrases are acceptable.

The patch is still green, back to RTBC.

Although.. the current patch does not address un-installing a module via the API (or drush). Is it ok to leave that for a followup?

1. Nice to see this really close!! Great work everyone

2. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,146 @@
   +      $this->formatPlural($count,
   +        'There is 1 @entity_type entity to remove.',
   +        'There are @count @entity_type_plural to remove.',
   +        [
   +          '@entity_type' => $entity_type->getSingularLabel(),
   +          '@entity_type_plural' => $entity_type->getPluralLabel(),
   +        ]
   +      )
   +      . ' ' .
   +      $this->t('This action cannot be undone.');
   +  }
   

   Let's not concatenate translations - this causes problems for RTL.

3. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,146 @@
   +          [__CLASS__, 'deleteContentEntities'], [$entity_type_id],
   ...
   +      'finished' => [__CLASS__, 'moduleBatchFinished'],
   

   Nice - I like keeping all the batch logic in the same place.

4. Have we considered permissions or are we assuming that whoever has the ability to uninstall modules should be able to delete content?

5. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,146 @@
   +      'progress_message' => static::t('Deleting @entity_type_plural... Completed @percentage% (@current of @total).', [
   +        '@entity_type_plural' => $entity_type->getPluralLabel(),
   +      ]),
   

   Where is @current, @percentage and @total coming from?

6. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,146 @@
   +    $entity_type_ids = \Drupal::entityQuery($entity_type_id)->range(0, 100)->execute();
   

   Maybe we should have the 100 as something configurable.

7. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,146 @@
   +    $context['finished'] = (int) count($entity_type_ids) < 100;
   

   This looks tricky - I think we should set it to finished when the number is 0 or the number has not changed from the last number. If the last number is not 0 then we need to fail and say that we couldn't delete something.

8. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,146 @@
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition($entity_type_id);
   +    $context['results']['entity_type_plural'] = $entity_type->getPluralLabel();
   

   I would just passed the entity_type_id into the results otherwise we need to this on every batch iteration.

I definitely think the Drush issue is fine to do in a followup - or even the drush queue.

I think this patch could do with a product manager review.

Re #69:

2. Fixed.
4. Yes.. I think that's a safe assumption :)
5. They were coming from _batch_process() itself, but I rewrote the progress message part to show a better overview of the real progress.
6. Done, made it configurable in the form, hidden under an 'Advanced options' details element.
7. We can only assume that we were able to delete everything, otherwise an exception would have been thrown and the batch would've stopped anyway.
8. Done.

I definitely think the Drush issue is fine to do in a followup - or even the drush queue.

It's not only Drush, it's our own module API that is not taken into account here, i.e. \Drupal\Core\Extension\ModuleInstallerInterface::uninstall(). Basically, the only reason that #874000: Automatically remove module's nodes upon module uninstallation is still open after so many years :)

However, the workaround for someone who wants to uninstall a module in an update hook or something is that they have to manually remove their content entities with Batch API, pretty much like how we're doing it in this patch.

1. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,169 @@
   +    // Inform the batch engine that we are not finished,
   +    // and provide an estimation of the completion level we reached.
   +    if ($context['sandbox']['progress'] != $context['sandbox']['max']) {
   

   One of the problems we have is that there is nothing to stop people creating content whilst this is on-going.

2. +++ b/core/modules/system/src/Tests/Module/PrepareUninstallTest.php
   @@ -0,0 +1,69 @@
   +    // Delete Node data.
   +    $this->drupalGet('admin/modules/uninstall/entity/node');
   +    $this->assertText(t('There are 4 content items to remove. This action cannot be undone.'));
   +    $this->drupalPostForm(NULL, [], t('Delete all content items'));
   

   Let's set this to do it one at a time then so we're sure that batching is working.

One of the problems we have is that there is nothing to stop people creating content whilst this is on-going.

True, but that only means that the user will have to go through this 'delete content' step again before uninstalling the module. It's not like we're automatically uninstalling it at the end of the batch process, we're just redirecting back to the module uninstall page.

Let's set this to do it one at a time then so we're sure that batching is working.

Sure thing :)

#76 looks good to me.

I agree that if content is created during the delete process then they will just have to run the process again. One would assume that users will only do this process on a dev version of the site too.

> I agree that if content is created during the delete process then they will just have to run the process again. One would assume that users will only do this process on a dev version of the site too.

The alternative is to run an ordered query for 100 entities in each batch run, and instead of setting the max at the start of the batch, set finished if the query returns fewer than 100. It means your batch UI can't show progress (or we can show approximate progress that might jump at the end).

I used this approach on a site with a batch that processed all entities of a certain type, while users were potentially adding more.

Will try and review this today during UX meeting. For now, adding the tags.

At a glance of the screen shots, though, it doesn't look like we're doing nearly enough here to inform people about what sort of destruction is about to happen. For example:

- Notice about 2 content items about to be deleted is not styled in any way (e.g. red error), so it's super easy to just click past it, leading to unwanted data loss.
- Rather than simply saying "2 content items" it probably should display a (truncated if necessary) list of node titles, which would more clearly communicate what is being deleted.

Will ping the UX team to see if they have other feedback.

@webchick: Note that the only way to get to this page is from the uninstall page when it tells you why a module can't be uninstalled. So a site builder shouldn't accidently end up there. But sure, adding a red message that this will delete data and that there is really no undo won't hurt ;)

Discussed this during UX meeting in Slack:

In general: not explicit enough about what is going to happen. We need to slow down this quite a bit (introduce more friction) so that people become really aware that there is no undo here.

1. For example: have people literally type in the word "Delete" in a text field before they can continue.
2. Showing a list of what's going to be deleted would also help estimate the impact. "3 Flagging entities" is quite abstract, can we show a list of titles instead? Similar to the confirm screen that follows bulk-deleting from the content listing:

3. Lets not make the primary button blue here, maybe we should introduce a big scary red button here.
4. Do we want to remind people to create a backup of their database first? That's the only undo option there is right?

So yeah, needs work :)

@yoroy - The patch extends ConfirmFormBase, and therefore follows all the design patterns of other confirmation forms. Should we apply your points from #82 to all confirmation forms?

@timmillwood: Well, the conclusion of the discussion was that most confirm forms don't have the potential to delete 5k nodes that you worked on for 4 years "accidentally". That this form can do such damage as to undo the majority of a site, so it needs to be treated as such.

Whatever the content admin page is doing to its confirm form, though, is a pattern that already exists in core, so however it's doing that, yes, we want to establish the same "OMG ARE YOU REALLY REALLY SURE?" pattern in both places.

The content admin page allows you to act on 50 nodes at most (by default). This form allows to act on all the content on the site, be it 50, 5000, 50000 or whatever. There is a clear level of difference in damage made. Not sure I would want to type in "Delete" each time I want to bulk-delete 2 nodes, but if I am about to delete four years of work, I would welcome the extra caution.

ok, how about, based on #82:
1) This confirm form
2) All confirm forms
3) All confirm forms
4) This confirm form

Btw, lest you think we're just being overly paranoid here, this same type of deal is literally how jQuery lost its entire plugins repository a few years ago, thanks to a major "oops" with VBO. :P https://blog.jquery.com/2011/12/08/what-is-happening-to-the-jquery-plugi...

#88: I think so, except I'd say 3) is probably this form only. Gábor is correct that this is pretty special, at least in terms of what ships with core.

Iterated over a design for this with @Bojhan, @gabor and @webchick:

1. Lets *not* do the type-something-to-confirm-deletion thing
2. List the first 10 titles of the entities about to be deleted
3. Mention the number of remaining items also about to be deleted. Show the actual number in bold
4. Add "Make a backup of your database if you want to be able to restore these items.
5. Make "Cancel" the default button, "Delete" a secondary red link

And by red Delete link I think what is meant is it should still be a submit button but look like a link. Otherwise page preloaders and other nifty client side tools would delete all your content :D

Cool, if that's the spec, my concerns are addressed!

Fine with me, just a note on the titles/labels. Not everything really has a label that can be shown there. Flaggings for example don't have a label, so the output there will be pretty weird.

I suppose we can skip the list if we detect that no labels are returned.

@Berdir makes a good point, I am here for ContentModerationState entities, which don't have labels. I guess we could compute one if that would be a good UX+.

Reversing the position of the action button and the delete seems like a really bad idea to me. People often go for UI elements based on their position without fully reading them. It s the reason OS X dialog boxes always have the confirm button at the bottom right, for instance.

Also, I think the name of the entity type needs to be in there somewhere. Titles can be ambiguous, and some entities won't have meaningful labels (eg I have no idea what flagging entities show for their titles). So either page title or the total count should include the entity type label in the sentence.

@joachim: the point of reversing the buttons was because people don't usually read them and this form can do massive irreversible damage to your site.

First of all: this is a great addition because it stops otherwise unnecessary searching round through the admin interface to find out where whatever comes from, and leads the user straight to the next task at hand.

Second: We don't actually have a separate style for situations in which the default option is to delete something. We got the default action as a blue button, a grey cancel button, and a red link for something destructive - but if the destructive action is the option you came to this page for then it's a blue button.
Since that is the same all over core, it probably doesn't make sense to discuss this here as an isolated event.

Third: In other cases when a user deletes not one item but several, they see a list of these items. So getting a list if you hit the "Remove content items" is the expected behaviour. In fact, I would expect a confirmation page that looks very similar to the one I get when I decide to delete the same items in a different way. (See screenshots.)

In this case it is even more relevant to see such a list, because what we delete here can go across a several entity types - labelled as something that is barely visible in the admin UI.
For example uninstalling the Node module, requires the user to delete content of the entity type: Content - and then it simply says all content items or remove x content items. Nowhere is the user alerted that this means deleting all pages and articles, or what ever custom entity types there are.

So there certainly needs to be a list of items. For deleting content through a bulk operation on the Content page, the length of the list is restricted by the number of items is shown in the View (50 items by default).
Following that: a list of items on the confirmation page can also contain 50 items. It could then have a line at the bottom of the list that says "and xxx other items."

To make this even safer, I would even propose to show the entity type in the list. That would make it really obvious.

• * Article: Lorem ipsum
• * Page: Veggie ipsum
• * Page: Yet another page
• * and another 123 content items.

Fourth: The action a user does is to delete something, and that's the word we use all over the place. Labelling the link "Remove" makes it sound like a different action (and possibly a less destructive one). So this should in any case say "Delete ...". Same in the help text on the confirmation page.

And fifth: yet another issue: So far we have one permission to administer modules, and that was fine in D7, when modules could be disabled. But since uninstalling modules is a destructive action now, we should think about making Uninstall modules a different permission. That would also make it less likely that somebody stumbles on the Uninstall page.

> Nowhere is the user alerted that this means deleting all pages and articles, or what ever custom entity types there are

That's a good point. Maybe in addition to 'X items' and the list of titles, we should give a summary like:

This will delete 123 content items, including:

- 43 articles
- 12 pages
- 35 issues
[etc]

> @joachim: the point of reversing the buttons was because people don't usually read them and this form can do massive irreversible damage to your site.

That is my point though. People don't read them properly, and someone could click the thing that LOOKS like a cancel button, but isn't.

@joachim: because of cancel buttons are red links? Or because of the spatial positioning of them? If we want to keep the button orders, colors, etc. because people may click the wrong thing out of habit, then it is hard to dismiss the idea to need to type in something explicitly into a text field.

The spatial position. I believe it's documented in usability studies that people often go by position of elements. I certainly notice that behaviour in myself. I can easily imagine coming to this page, reading the warnings, thinking 'oops, that doesn't look good' and then clicking what I automatically think is the cancel button but without reading the text of it. (Of course myself I'd just close the tab...)

I think the delete button in red in the usual position would be much better.

How about keeping the red delete button on the left but disabled, and add a checkbox above it "I'm sure I want to delete all items", which has to be checked in order to enable the delete button?

Edit: at least this makes it a two-click process, so the user is less likely to do both actions by accident.

For now, this patch implements the design that was agreed upon in #90:

> How about keeping the red delete button on the left but disabled, and add a checkbox above it "I'm sure I want to delete all items", which has to be checked in order to enable the delete button?

Yup, that sounds good to me. Potentially a UI pattern we could abstract out later on.

I prefer not to change direction again. Both a red button and a checkbox have significant implications, and should warrant further discussion.

Lets move ahead with the direction which was agreed upon. We can easily optimise most of this later on and right now its holding up a big initiative from going in. This is experimental, we can change it after commit.

Can you still change the wording to talk about "Delete" instead of the weaker "Remove"?

@Bojhan: to be precise the patch changes various existing parts of the stable Drupal core and therefore is not experimental. The functionality it blocks is experimental but not this patch. That does not mean we cannot make changes to it after the patch lands but much less so to the API at least after 8.2 is released.

> Both a red button and a checkbox have significant implications, and should warrant further discussion.

Fair enough. I'd be happy to leave those for later.

However, I do think that the reversed buttons is a big UX mistake.

Fixed and improved the tests for the new UI.

1. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -37,10 +38,14 @@ public function __construct(EntityManagerInterface $entity_manager, TranslationI
   +          '@entity_type_plural' => $entity_type->getPluralLabel(),
   

   Nice a usecase for the plural label

2. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -37,10 +38,14 @@ public function __construct(EntityManagerInterface $entity_manager, TranslationI
   +          ':link' => Url::fromRoute('system.prepare_modules_entity_uninstall', ['entity_type_id' => $entity_type->id()])->toString(),
   

   Maybe I nitpick and say that this is a URL not a link :P

3. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,231 @@
   +      '#type' => 'number',
   +      '#title' => $this->t('Limit'),
   +      '#description' => $this->t('Select how many items should be deleted at once in a single batch run.'),
   +      '#default_value' => ($count < 100) ? $count : 100,
   +    ];
   

   Am I the only one who things that nobody actually has to care about this number? Deleting an entity throws hooks is much more costly than having x or 10x batch jobs, so I would have just gone with 10 entities or so and call it a day? The batch API deals with starting new processes, when time is gone.

4. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,231 @@
   +    if (!isset($context['sandbox']['progress'])) {
   +      $context['sandbox']['progress'] = 0;
   +      $context['sandbox']['max'] = \Drupal::entityQuery($entity_type_id)->count()->execute();
   +    }
   

   Do we care about content which is created in the meantime?

5. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,231 @@
   +    $entity_ids = $storage->getQuery()->range(0, $limit)->execute();
   

   should we sort by some criteria like ID descending or ascending?

6. +++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
   @@ -0,0 +1,231 @@
   +    if ($entity_types = $storage->loadMultiple($entity_ids)) {
   +      $storage->delete($entity_types);
   +    }
   

   Weird variable name ... can't we just use $entities

7. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
   @@ -37,16 +36,6 @@ protected function setUp() {
      /**
   -   * {@inheritdoc}
   -   */
   -  public function register(ContainerBuilder $container) {
   ...
   -    // Put a fake route bumper on the container to be called during uninstall.
   -    $container
   -      ->register('router.dumper', 'Drupal\Core\Routing\NullMatcherDumper');
   -  }
   

   I love the typo in here

Thanks for reviewing!

Re #111:

1. Yup! :)
2. You would be right, fixed.
3. I don't really care either, but @alexpott wanted it to be configurable :)
4. Nope, we don't care that much, see #75/#76.
5. Why not, let's make sure that we delete the oldest first.
6. Yup. fixed.
7. ;)

Also fixed a typo and a comment.

I seem to be able to break things.

1. Apply patch
2. Install standard
3. Install devel_generate
4. Create 1000 terms (drush generate-terms tags 1000)
5. Log in as user 1
6. Go to admin/modules/uninstall/entity/taxonomy_term
7. And try to remove them

It gets stuck :)

This is happen because of deleting parents - see \Drupal\taxonomy\Entity\Term::postDelete()

Here's a simple fix.

That's a very simple fix indeed, @alexpott++

Is the patch in #116 supposed to work on simplytest? I enable forum, create forum node, then go to uninstall forum, which then still has a disabled checkbox, telling me to delete forum nodes first.

@yoroy well forum does not provide the entity type - the node module does.

Yeah, that's a slightly different use case because forum only provides a node type/bundle for nodes.

I think we can relatively easily extend this to support bundles as well, we just need a condition in the entity queries and pass that argument along, but I think we should do a separate issue for that.

I agree with @Berdir - let's do that in a follow-up.

This patch looks to be in a good state, and as it's blocking #2725533: Add experimental content_moderation module, lets get it RTBC'd.

+++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
@@ -0,0 +1,237 @@
+    $form['actions']['cancel']['#weight'] = -1;

This reordering of the buttons is a really big mistake.

Unrelated #fail.

Before we commit this:

• I think we should add test coverage (using taxonomy terms) for the bug fixed in #116
• we should create the followup for #121
• And we need a change record to tell everyone about this functionality

Here's a test for the bug fixed in #116, also applied to the patch from #113 to prove that it's failing.

Wrote a draft CR for this feature: https://www.drupal.org/node/2772525 and this is the followup requested for #121: #2772511: Support uninstalling modules that provide bundles for content entity types

Nice test.

+++ b/core/modules/system/src/Tests/Module/PrepareUninstallTest.php
@@ -41,12 +49,53 @@ public function setUp() {
+    // disabled.

uninstalled :) can be fixed on commit.

Just re-read #111.3 and realised I caused the additional form element - what I was concerned about was properly testing this whilst batching. The new taxonomy test help resolve this concern. I've removed it and bumped up the number of taxonomy terms created. I've hard-coded the limit to 10.

Hm. 10 is a pretty low value IMHO. I guess that's set so low for the term recursive deletion, but even when setting it to 1, there's not really a limit to how many terms it might actually delete?

The problem is that batch overhead is quite high, and when deleting just 10 per batch run, I guess you spend more time on refreshing and bootstraping than actually doing something (batch calls multiple operations but only until 1s is passed).

Just pointing that out, I care way more about actually getting this in than fighting over the limit :)

Am I the only one who things that nobody actually has to care about this number? Deleting an entity throws hooks is much more costly than having x or 10x batch jobs, so I would have just gone with 10 entities or so and call it a day? The batch API deals with starting new processes, when time is gone.

I think if you're doing 1000's of entities we should provide an integration with drush.

Fixing the disables

The problem is that batch overhead is quite high, and when deleting just 10 per batch run, I guess you spend more time on refreshing and bootstraping than actually doing something (batch calls multiple operations but only until 1s is passed).

Well, it still calls operations until you get near the 1s mark, doesn't it? So the overhead of batch operations should be kind of small over what happens on entity deletion ...

Discussed a bit with @alexpott on IRC and we decided to mark \Drupal\system\Form\PrepareModulesEntityUninstallForm::deleteContentEntities() as @internal in order to make it possible to add additional parameters to it (e.g. $limit or $bundle) in the future without having to worry about BC.

Thanks for creating that followup. Quite likely this design can be improved further still, at least we've made an explicit decision here.

Pity the ui issues surfaced a bit late but i'm happy we accomodated for that bit of work to happen still.

Lets go ahead with this!

+++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
@@ -0,0 +1,223 @@
+          '#markup' => $this->t('And <strong>@count</strong> more @entity_type_plural.', ['@count' => $count - count($labels), '@entity_type_plural' => $entity_type->getPluralLabel()]),

Hmmm this should be a formatPlural... because saying "And 1 more content items" does not make sense.

Patch attached:

• Fixes #142
• Makes the form result in a 404 when passed an entity type that does not exist
• Removes the submit button and tells the user why when there are no entities to delete
• Tests the no label key situation
• Tests all the new changes

+++ b/core/modules/system/src/Form/PrepareModulesEntityUninstallForm.php
@@ -91,6 +92,9 @@ public function getCancelUrl() {
+    if (!$this->entityTypeManager->hasDefinition($this->entityTypeId)) {

Maybe we should also add a check for a non-null $entity_type_id at the beginning of this condition. Or is the routing system handling that for us?

This is pretty minor and can be fixed on commit, the rest of the interdiff looks great :)

Maybe we should also add a check for a non-null $entity_type_id at the beginning of this condition. Or is the routing system handling that for us?

This is pretty minor and can be fixed on commit, the rest of the interdiff looks great :)

Well I would not fix anything more than a coding standards or docs fix on commit, personally. :)

While testing this patch to potentially commit it, I accidentally canceled the form the first time instead of deleting the items, because the Cancel button was where the normal "Yeah do this thing" button would be.

I went back and confirmed that I am not crazy; it is different from every other confirm form, including the one that you might have to visit right before you do this (in my case, uninstalling Taxonomy and History in order to be able to uninstall Node and test the batch deletion).

If I can accidentally cancel my form because the button moved, it's not inconceivable that someone might accidentally click the link when they want to cancel. Less likely, perhaps.

I see this design was discussed earlier on the issue, and @Bojhan suggested going forward with it because it was an experimental module and we can iterate on the best design later. The thing is, this is not an experimental module. This is a change to stable core that lets you delete all your content.

Are we really, really, really sure that basically xjm is just an idiot who needs to pay more attention, and that it is better to have the new design for this one form and no other?

I should add that the functionality itself seems to work great, the listing on the confirm form makes it clear what the consequences of the action should be, and the hardcoding at 10 items in the batch job seemed fine based on my testing with large-ish batches (admittedly without 20 other modules firing delete hooks, but nonetheless). I will go ahead and commit it if the usability maintainers say "yes absolutely really". But in the earlier discussion, it did not seem to me that the full consequences of this were clear. Edit: And the first user to test it seems to have gotten it wrong, soooo... :P

I'm curious where the missing conversation is regarding:

For example: have people literally type in the word "Delete" in a text field before they can continue.

I agree with @xjm's comment in #146. I think the most relevant example is the uninstall screen where we are listing configuration entities that will be deleted. We have not changed the pattern there - so changing the pattern here seems wrong too. I think we need to remember that the only people who can get to this screen are people who can uninstall modules - which is a very data destructive operation already. Maybe the way forward here is to make this form a usual confirm form and open a followup issue to discuss the design of super-destructive confirm forms like this and the module uninstall form - so we can be consistent.

Given that this is critical for getting workflow in. I have no objects to removing the change to confirm forms and create a propper followup to solve this. This is a tricky problem and needs a consistent solution.

I am very concerned about all the WI Criticals that get held up though :(

Okay removed the special button styling...

Created the follow up #2773205: Come up with a design for highly destructive operations in confirm forms

Re #144 - visiting admin/modules/uninstall/entity results in the expected 404 - we don't need to check for NULLness. But also...

>>> \Drupal::entityTypeManager()->hasDefinition(NULL)
=> false

So all good imo.

I agree we can go ahead with this in the interest of unblocking workflow. I'm confused why #146 is the rationale, though. The accidental click resulted in a non-destructive action, forcing her to go back and read the actual options, which is exactly what it was intending to do.

For me the rationale is we already have super-destructive operations that precisely a user with the perms necessary to get here and we don;t special case that form either. I do think we should consider the UX pattern for such forms but the arguments that @joachim put forward about spatial reasoning are strong - after all the accidental click documented in #146 could have been intended to cancel but instead deleted everything.

DrupalCI is busy fixing itself.

@twistor: We discussed that alternate pattern briefly in UX meeting the other week, and it was only really supported by Kevin. Bojhan had done some user-testing of it and found it to increase user frustration, and overall feels like a "cover your ass" cheat, rather than an actual design, IMNSHO. :) But something we can explore in the new sub-issue.

Reviewed this upon @alexpott's request, as well as manually tested it. Couldn't find anything to complain about!

Committed and pushed to 8.2.x. Thanks!

I created the followup #2773205: Come up with a design for highly destructive operations in confirm forms