Deleting a Flag doesn't delete flaggings

Doesn't core delete the flaggings when the flag is deleted? Or do you mean that we need to do more than broadcast a FLAG_DELETED event?

So this is my attempt to clarify the situation.

Step to expose the problem..

1) Create one flag ( for articles )
2) Create 2 articles
3) Flag both.

As expected two row in the flagging table, 2 rows in the flag_count table.

4) Delete flag

Reinspect the database - both tables are unchanged.

This is a first draft... codifying the steps to reproduce ( see #3 )

I will have more time to work on this Sunday...

FlagCountTest is maybe not the place for this but hey this is the initial version.

The next iteration. - the tests are unmodified, I have just fixed the module so that they pass.

Following the pattern we developed in FlagCountManager
FlagService now subscribes itself to FlagDelete events and handles the resetting of all flags in turn.

So making notes while it is fresh in my head

Flag::preDelete() dispatches a FLAG_DELETE event
which invokes FlagService::reset() which dispatches a FLAG_RESET event
which invokes FlagCountManager::resetFlagCounts

so all the database tables that must be touched are updated ( see #3 )

The test as they stand do not confirm that the flag_count_table has been cleared, but hey this is work in progress.

FlagService::deleteHandler() seems like a bad name ... any suggestions welcome :)

PS I have tried being more descriptive in the FlagService class description .. that will need review!

1) Change some text as events can be misinterpreted in that context.
Performs flagging and unflaging eventsoperations.

2) I have been more complete about checking the state of the count and flaggings before and after the delete.
Nothing more than making the test follow the manual steps outlined in #3

3) The FlagCountService is used in both tests .. so it is now defined in setup()

4) Various coding standard nit piks as identified by phpcs

5) Updates issue summary, because I think I have shown there is no need for an API change.

I think this issue is now complete.

1. +++ b/src/FlagService.php
   @@ -314,4 +319,23 @@ class FlagService implements FlagServiceInterface {
   +  public function deleteHandler(FlagDeleteEvent $event){
   

   deleteEvent() maybe? "handler" is a bit too generic.

2. +++ b/src/Tests/FlagCountsTest.php
   @@ -55,6 +61,7 @@ class FlagCountsTest extends FlagTestBase {
   +    $this->flagCountService = \Drupal::service('flag.count');
   

   Shouldn't the Count Service be added to FlagTestBase? That's a debate for another issue, though.

Other than that, it looks good.

Thanks for the review.

For me deleteEvent() chimes too closely with an existing class name \Drupal\flag\Event\FlagDeleteEvent

I am too close to sleeping to come up with something definitive, but something descriptive.

Maybe a mix and match from the following possibilities

clearTotalsOnFlagDelete()
resetOnFlagRemoval()
onDeleteResetCount()

Just trying this name on for size.

onDeleteResetFlaggings()

Drupal core HEAD seems to be down at the moment....so I won't move this to Needs Review.

Just looking over the tests again, there is no need to create a user who may flag.

I am re-testing now that drupal core is up.

Much better! I think this is ready to go.

Given that issue has sat fallow for 2 years, I'm not confident in holding up this issue for a change in core. Meanwhile, the problem of flaggings not being deleted is making the module uninstallable.

So I suggest we go forward with this patch, even if it does have performance implications, and then create a separate major issue to use the queue API for flagging deletion when/if it becomes available.

+++ b/flag.services.yml
@@ -8,6 +8,8 @@ services:
+    tags:
+      - { name: event_subscriber }

I'm bikeshedding, but I wonder if we should break off the deletion management into a separate FlaggingDeletionService instead of putting this into FlagService. We could later refactor FlaggingDeletionService into FlaggingService -- an idea that was banded about in other issues.

That does have the advantage that people can choose their deletion-related performance issue more easily :p

I know it sucks, but a separate service compartmentalizes the problem so we can more easily fix it the Right Way later.

I know it sucks,

I was thinking the opposite... I like stepping stones... -- I wish I had thought of that. :)

I have separated out the FlaggingDeletionService.

The best place, I found, for the "beware of the dog" notice expressing concern about scaling is in the FlagDeletionService class definition.

It is not bug free... but the overall structure is there...
more tomorrow :)

The failing tests now pass..

As far as the timeline for

#89181: Use queue API for node and comment, user, node multiple deletes

The earliest opportunity for that improvement is 8.1, so 6 months after release candidate becomes 8.0...hmmm

That works, but I was really hoping to only have the event subscriber moved to a separate service, not the reset() method. Since reset() is an API function, I'd rather keep it in FlagService with the rest of the API.

Under that scheme, FlagService::reset() would really only dispatch the reset event. FlaggingDeletionService would do the actual deleting, then reply by setting the flagging count on the event. Then FlagService::reset() would return.

All this sounds rather overcomplicated, though. :-/

Since reset() is an API function, I'd rather keep it in FlagService

Fair enough...

So the next patch should start from #12 ...

That would make the new service a single method class which makes calls out to another service to function... can I suggest that looks like a dedicated event subscriber. not a service.

I am busy now until the weekend ... but I will get to this.

Yeah.... I kinda bikeshedded myself into a corner with #26.

Possible compromise: Instead of FlagDeletionService, we just name it FlaggingService from patch #24. And I can take my high ideals and bury them in the backyard somewhere. ^_~

Renamed the service to FlaggingService.

The change works for me...

The class property FlagCountsTests::flaggingDelete

@var still mentions

* @var \Drupal\flag\FlaggingDeleteServiceInterface

I am short on time today ... I will get to this tomorrow...

Fixed the comment.

This kind of depends on #2477783: is FlagDeleteEvent necessary?, I think? Because if that one gets merged in, then this one is referencing an event that no longer exists. So, assuming that #2477783: is FlagDeleteEvent necessary? is going to get approved and merged in, this would need work.

Here's an updated patch!

Fixed a missing newline in the the process. Committed! Thank you!