impossible to uninstall Flag

But because there is no single UI to delete all Flaggings...

There isn't a single "nuke everything" button, but you can delete all the flaggings for a flag from the admin UI now: #2491489: Create flag reset admin operation

We could create another action on the admin UI that deletes everything, but I'm unsure how many flags (not flaggings) are on the average site.

Yeah, you'd think that. I didn't find anything given my admittedly brief search last night.

There is a perfect world and then "lets nuke it from orbit - just to be sure"
At some point that big red button always comes in handy....

BUT I think we should postpone this issue until

#2489072: Deleting a Flag doesn't delete flaggings

The rule for all modules :-
Impose a gate... until all content (I mean flags) associated with this module is deleted.... then you can remove the module.

IF that patch gets accepted then flaggings will be removed with the flags and everything falls into line with the normal conventions.

Setting back to Active now that #2489072: Deleting a Flag doesn't delete flaggings is fixed.

It's not great, but we solved this in simplenews for now by adding a "delete-all-stuf-and-prepare-uninstall" form that uses batch API to delete all content and also remove all problematic configurations like fields from this module.

See #2418659: Can not uninstall: "Fields type(s) in use" for inspiration.

Thank you!

Oh, definitely. But I have no idea when that's going to happen and we'll have to live at least the next 6 month with what we have now.

Want to open a core issue to add something like a [Purge all data] button to the core uninstall UI? Possibly in the form of a confirm page.. so it would say like, "You want to uninstall flag module, which currently has 25'356 flaggings stored, confirm that they will be deleted" and then it just does.

Oh, definitely. But I have no idea when that's going to happen and we'll have to live at least the next 6 month with what we have now.

Agreed. The functionality isn't too difficult to implement, and the batch code could be reused for deleting a single flag if coded properly. Right now we brute-force the deletion as a stop-gap in FlaggingService::reset().

Another possible solution to this would be to create a "prep uninstall" module. The module's only job is to allow you to delete custom entities created by a module. Apparently no one has a module named "nuke" yet...

Making some progress with this. Patch forthcoming.

Still needs tests, but it does appear to work in my local installation and make the module ready for uninstallation.

There's a few compromises with this approach:

• The flags are deleted at the storage level without broadcasting any events or using the Flag Service.
• The flaggings are reset, one flag at a time instead of deleted at the storage level.

Ideally, we should move the batch operations to the Flag and Flagging Services, but that can be filed in another issue.

Removed a unnecessary comment and fixed the newlines.

*shakes fist at PHPStorm*

Added some missing docblocks.

I'm concerned about scaling here. We're doing one flag per batch op. What if one of the flags has a million flaggings? It would be better to iterate over flaggings, doing say 50 at a time.

That's a good idea. I avoided it initially because I didn't want to deal with clearing the counts table manually as well. There's also the meta-issue that FlagServiceInterface::reset() really should be using a batch process itself. I had hoped this issue would be a stepping stone toward that solution.

Fixes minus the change to UninstallForm::resetFlags().

Changed the behavior of ::resetFlags() to delete the flaggings directly. I've also augmented the tests to check the size of the flag_counts table.

What I don't understand is how the table is empty after deleting the flaggings, even though I don't clear the table in the UninstallForm.

It can be difficult digging through the comments to find the salient points in the discussion. I'm moving stepping stone argument from #21 into the issue summary ... otherwise I know I will forget.

1. +++ b/src/Form/UninstallForm.php
   @@ -0,0 +1,147 @@
   +      'title' => t('Deleting Flag data'),
   

   $this->t()

2. +++ b/src/Form/UninstallForm.php
   @@ -0,0 +1,147 @@
   +      'progress_message' => static::t('Deleting Flag data...'),
   

   I see you copied this from simplenews, but I think this should be $this->t(), no idea why this even works :)

3. +++ b/src/Form/UninstallForm.php
   @@ -0,0 +1,147 @@
   +    // First, set the number of flags we'll process each invocation.
   +    $batch_size = 15;
   

   Thats seems like a rather low number. Deleting a flag seems like it should be a fairly fast operation, there won't be a lot of fields in 99% of the cases and only a single table (no revision/translation tables).

   So I think you can easily go with 50 or 100 here. Possibly even more. Keep in mind that Drupal calls this repeatedly up until 1 second is over and then it has to do a roundtrip through the browser and another bootstrap etc. Larger batches means fewer requests and overall faster progress.

   Especially if you do what I'm suggesting below.

4. +++ b/src/Form/UninstallForm.php
   @@ -0,0 +1,147 @@
   +    foreach ($storage->loadMultiple($ids) as $entity) {
   +      $entity->delete();
   +    }
   

   You can do $storage->delete($storage->loadMultiple($ids)) then the storage can do a single DELETE query for all and also invoke a single hook.

(Still can't believe we have to do this in D8...)

I agree, lets open a core issue ;)

+++ b/src/Form/UninstallForm.php
@@ -0,0 +1,147 @@
+  /**
+   * Batch method to delete all flags.
+   */
+  public static function deleteFlags(&$context) {
+    // First, set the number of flags we'll delete each invocation.
+    $batch_size = 1;
+

this should not be needed. core happily deletes config automatically on uninstall.

Tore out all the config entity deletion, since it wasn't necessary.

Added a table truncation. It's over-engineered, since it pulls from the hook_schema() definitions and deletes any tables the module defined. Still, it'd be a good example for other modules.

Hm, not sure why you did that? the only problem are content entities (and fields of a type provided by the module itself but flag doesn't have that). Normal tables are deleted just fine.

Hm, not sure why you did that?

Not sure why I did either, now that I think about it. I removed the code to truncate the schema.

1. +++ b/flag.links.task.yml
   @@ -2,3 +2,13 @@ entity.flag.edit_form:
   +flag_uninstall_tab:
   +  title: 'Uninstall'
   

   The title and form class name are a bit confusing, it's uninstalling flag, it's preparing for it.

2. +++ b/src/Form/UninstallForm.php
   @@ -0,0 +1,102 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDescription() {
   +    return $this->t('This will delete all flags, flaggings, and all Flag data. This operation cannot be undone.');
   +  }
   +
   

   This should be updated. I'd also mention that this is about preparing to uninstall somewhere in this form.

Renamed UninstallForm and Uninstall tab to ClearAllForm and "Clear all".

Re-added table schema truncation. It is possible that someone may use the clear all tab not to prepare for uninstallation, but just to clear everything for some other reason. In that case, the counts tables will be wrong. The tests have been augmented to test the counts table for this case.

Augmented the form completion message to link to the module uninstall page.

I personally wouldn't bother with truncation for those tables, but I won't have to maintain the code, so I don't really care :)

I just want to get the criticals resolved so we can release this. Setting to RTBC so that @joachim can have a look at it.

I just want to get the criticals resolved so we can release this.

Same. Since this has sit for over a week without further comment, let's get it in. Any further problems can be resolved in new issues.