Update flag counts in hook_ENTITY_TYPE_insert/delete, rather than in Flag's API

It's starting to look that way. Fortunately, we can broadcast flagging events from anywhere, so it shouldn't be hard to move that logic from FlagService to the entity classes instead.

Why not Entity::postSave() for flagging, and Entity::preDelete() for unflagging? Why go to the hook?

Instead of postSave(), ContentEntityBase::postCreate() would be better.

That would explain why it blows up impressively and doesn't behave as expected. >_<

There's a bigger problem with preDelete() in that the flagging entity no longer seems complete. There's a null flag and flagging.

So from https://www.drupal.org/node/2705789#comment-11084445

A blocker of a critical, becomes critical...

I don't think it makes a difference to the failures, but *shrug*.

Now that's interesting. If we change our delete hooks to predelete hooks, now we're only getting odd counts instead of crashes.

This one only fails when resetting the flag, deleting a node, or deleting a user.

So does that mean the if statement in FlagService unFlagAll() can safely be removed, as flagging->delete() will always fire the event?

  public function unflagAll(EntityInterface $entity = NULL, AccountInterface $account = NULL) {
    $flaggings = $this->getFlaggings(NULL, $entity, $account);

    /** @var \Drupal\flag\FlaggingInterface $flagging */
    foreach ($flaggings as $flagging_id => $flagging) {
      // We're working around the potential situation where the flagging
      // is already gone and the unflag event won't get fired.
      if ($entity ? $flaggable = $entity : $flaggable = $flagging->getFlaggable()) {
        $this->unflag($flagging->getFlag(), $flaggable, $account ? $account : $flagging->get('uid')->entity);
      }
      else {
        $flagging->delete();
      }
    }
  }

BLOODY TYPO!

1. +++ b/src/Entity/Flag.php
   @@ -598,6 +598,18 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {
   +    foreach ($entities as $flag) {
   +      $flagging_service = \Drupal::service('flagging');
   +      $flagging_service->reset($flag);
   

   You can move the first line out of the loop.

2. +++ b/src/Entity/Flagging.php
   @@ -182,4 +184,31 @@ class Flagging extends ContentEntityBase implements FlaggingInterface {
   +      $flag = $this->getFlag();
   +      $entity = $this->getFlaggable();
   +
   +      \Drupal::service('event_dispatcher')->dispatch(FlagEvents::ENTITY_FLAGGED, new FlaggingEvent($flag, $entity));
   

   Not something for this issue, but isn't it a bit strange at this point that FlagginEvent doesn't just have the Flagging and you can get what you need from that? We could still over getFlag() method on it directly, but internally, it would just have a single property..

Not something for this issue, but isn't it a bit strange at this point that FlagginEvent doesn't just have the Flagging and you can get what you need from that? We could still over getFlag() method on it directly, but internally, it would just have a single property..

I tried that once, but I thought I hit a block there. Maybe I'm misremembering.

Refactored FlaggingEvent to take...you know, a flagging.

+++ b/src/FlagCountManager.php
@@ -155,11 +155,19 @@
   public function incrementFlagCounts(FlaggingEvent $event) {
+    /* @var \Drupal\flag\FlaggingInterface flag */
+    $flagging = $event->getFlagging();
+
+    /* @var \Drupal\flag\FlagInterface flag */
+    $flag = $flagging->getFlag();
+    /* @var \Drupal\Core\Entity\EntityInterface $entity */

Those @vars are not necessary. You already type hint on FlaggingEvent, so you know what getFlagging() returns and in turn, that knows what getFlag() and getFlaggable() returns.

If that's not the case then we need to fix the documentation of those methods.

Berdir mentioned on IRC that now we have a double-delete problem when we issue a flag reset. The patch swallows the error, but we really do need to fix this.

Is there a way we can leverage the FlaggingStorage class to solve this problem?

Removed unnecessary @vars.

Included changes from https://www.drupal.org/node/2704959#comment-11092407

Just a minor tweak while I see it

I am moving a \Drupal::service() call out of a loop.

it will only be significant if someone deletes a bucket load of flags.

Dammit. Missed that one. >_<

This somewhat contradicts #2704959: FlagCountManager::decrementCounts() doesn't handle empty results gracefully now. Because with this, that scenario will definitely happen every time a flag is reset.

As mentioned in IRC, there are kind of two ways forward:

a) Ignore that problem for now, do *not* commit the other issue, just avoid errors. We should however at least avoid doing a bogus db update query on something that won't ever do anything, meaning, we need a third case for count 0 that does nothing.
b) Fix it here by removing the flag reset event. It will make it a bit slower as each removed flagging will decrement the counts on its own.

Either way, I think the flag reset API needs to be removed entirely. Just provide a batch that deletes all flaggings, just like the prepare uninstall stuff. It's impossible to make an API that scales.

Also, the flag API currently accepts an entity and will only reset flags for that entity. The event however doesn't contain that information and the flag counts will all be removed.

It also occurred to me to change the way preDelete() operates. It can receive multiple flagging deletes. Instead of reset, we create a new FlagBatchDelete event that can accept multiple deletes in place of reset. Then if only one flagging is passed to preDelete(), we issue a regular unflagged event.

Yes, I was thinking around that as well.

But do you really want to do two different events? Then anyone who cares about that needs to implement both anyway. I'd just change unflag to always support multiple flaggings. flag can probably stay single, as there's no API to save multiple entities at once anyway.

The decrements method can get a bit complicated then, unless you just want to loop over all flags, but then we don't actually benefit from having multiple. I think you can do something like that:

1. Loop over all passed flaggings, build a list of flags, entity types and entity ids., also a count of flaggings per flag/entity_type/id.
2. Do a query for an IN for each of those, group by flag, entity type and id (The thing is, it's always only one entity type per flag, so we could just leave that out, not sure?).
3. Loop over the results (this way, you automatically skip any that don't exist), if you have the same count of flaggings being deleted ( or less?) delete, if there are more, update and decrement by the count of flags that are being deleted.

Then you just have to update the reset method to call $storage->delete($flaggings) and remove the reset event. Still needs to be converted to a batch but then we can do that without an API change.

Then anyone who cares about that needs to implement both anyway. I'd just change unflag to always support multiple flaggings. flag can probably stay single, as there's no API to save multiple entities at once anyway.

That seems sensible.

It looks like there is a branch merging bug with

#2707127: Flag::toArray() is no longer needed.

so Flag config_export is corrupted - and Flag::toArray() reappears.

Can't quite figure out why I can't get the entity type manager to inject into FlaggingService without it blowing up.

Fixed typo, deleted reset event completely.

Whitespace fix.

1. +++ b/src/Entity/Flag.php
   @@ -54,8 +54,6 @@ use Drupal\flag\FlagInterface;
     *     "link_type",
   - *     "flagTypeConfig",
   - *     "linkTypeConfig",
     *   },
   

   this looks like a bad merge conflict, this shouldn't be reverted?

2. +++ b/src/Entity/Flag.php
   @@ -597,4 +595,36 @@ class Flag extends ConfigEntityBundleBase implements FlagInterface {
   +    // @todo Do we need Flag::toArray() any longer?
   +    $properties = parent::toArray();
   +    $names = [
   +      'flag_type',
   +      'link_type',
   

   this is part of the the same conflict.

3. +++ b/src/FlagCountManager.php
   @@ -177,36 +182,55 @@ class FlagCountManager implements FlagCountManagerInterface, EventSubscriberInte
   +    $flaggings = $event->getFlaggings();
   +    foreach ($flaggings as $flagging) {
   +      // Get the flag and flaggable.
   

   I see that you're not yet implementing my idea above. I guess it can be a follow-up, but it's fairly important to speed up resets of a lot of flaggings.

4. +++ b/src/FlagCountManager.php
   @@ -177,36 +182,55 @@ class FlagCountManager implements FlagCountManagerInterface, EventSubscriberInte
   +        $message = t('Decremented null count on flag @flag on @entity with type @type', [
   +          '@flag' => $flag->id(),
   +          '@entity' => $entity->id(),
   +          '@type' => $entity->getEntityTypeId(),
   +        ]);
   +        throw new \LogicException($message);
   

   exceptions shouldn't use t()

5. +++ b/src/FlagCountManager.php
   @@ -216,7 +240,8 @@ class FlagCountManager implements FlagCountManagerInterface, EventSubscriberInte
       */
      public function resetFlagCounts(FlagResetEvent $event) {
   -    /* @var \Drupal\flag\FlaggingInterface flag */
   +
   +    /* @var \Drupal\flag\FlagInterface flag */
   

   this method can go.

6. +++ b/src/FlagService.php
   @@ -276,14 +272,7 @@ class FlagService implements FlagServiceInterface {
        /** @var \Drupal\flag\FlaggingInterface $flagging */
        foreach ($flaggings as $flagging_id => $flagging) {
   

   this can also change to a $storage->delete($flaggings)

7. +++ b/src/FlaggingService.php
   @@ -58,33 +62,24 @@ class FlaggingService implements FlaggingServiceInterface {
       */
      public function reset(FlagInterface $flag, EntityInterface $entity = NULL) {
   -    $query = $this->entityQueryManager->get('flagging')
   -      ->condition('flag_id', $flag->id());
   -
   -    if (!empty($entity)) {
   -      $query->condition('entity_id', $entity->id());
   -    }
   -
   -    // Count the number of flaggings to delete.
   -    $count = $query->count()
   -      ->execute();
   -
   -    $this->eventDispatcher->dispatch(FlagEvents::FLAG_RESET, new FlagResetEvent($flag, $count));
   -
        $flaggings = $this->flagService->getFlaggings($flag, $entity);
   -    foreach ($flaggings as $flagging) {
   

   I'm not really sure I see the point in the flaggingService, especially given that getFlaggings() is actually on flagService and this is now just an entity query + delete. Which will probably go away completely when switching to a batch process for reset.

8. +++ b/src/Tests/FlagCountsTest.php
   +++ b/src/Tests/FlagCountsTest.php
   @@ -1,17 +1,19 @@
   
   @@ -1,17 +1,19 @@
    <?php
   -namespace Drupal\Tests\flag\Kernel;
   +/**
   + * @file
   + * Contains \Drupal\flag\Tests\FlagCountsTest.
   + */
   +
   +namespace Drupal\flag\Tests;
    
   

   Just like the config_export stuff, this all seems like an accidental and unnecessary revert of the previous commit? there's no reason that this has to be changed back.

Still missing the speed enhancements from #34.

Implemented the suggested decrement method. Went a bit overboard, but the good news is that deleting N flaggings of one flag will result in just two queries now. One select and one delete.

Also some minor cleanup.

+++ b/src/FlagCountManager.php
@@ -168,66 +172,83 @@ class FlagCountManager implements FlagCountManagerInterface, EventSubscriberInte
+      $flag = $flagging->getFlag();
+      $entity = $flagging->getFlaggable();
+
+      $flag_ids[$flag->id()] = $flag->id();
+      $entity_ids[$entity->id()] = $entity->id();
+      if (!isset($flaggings_count[$flag->id()][$entity->id()])) {
+        $flaggings_count[$flag->id()][$entity->id()] = 1;
+      }
+      else {
+        $flaggings_count[$flag->id()][$entity->id()]++;
+      }
+
+      $this->resetLoadedCounts($entity, $flag);

That's...a lot of calls to id(). Can we get the ids up front (instead of the entities), and then pass the entities inline to resetLoadedCounts() by calling getFlag() and getFlaggable()?

That's my own nit to pick.

I don't care about $flag_id vs. $flag->id(), personally, I find that there is little difference in readability between the two.

@joachim:

Use reset with rules how exactly? Do something when a flag is reset or trigger a flag reset? The second is not a problem, the API still exists, it's simply a "delete all flaggings" now (like it already was before). That is a scalability issue in case you have a lot of flaggings, but as I wrote above, the only way to make it scale with content entities that might have fields is to make it use batch. It's also not more of a scalability issue with this patch than without.

Reacting to a flag being deleted would be harder, you could currently check if the flaggings list includes all list of a flag. We could also keep the event and not use it for now. But the way it is currently implemented is broken in some cases, for example when only a single entity is reset. the event right now doesn't have that data. So instead of keeping it, we could do a follow-up to consider re-adding it?

I don't care about $flag_id vs. $flag->id(), personally, I find that there is little difference in readability between the two.

It's a personal preference. *shrugs*

So instead of keeping it, we could do a follow-up to consider re-adding it?

We could keep reset as a courtesy event. It can be fired from the reset() API method, but we do not use internally to decrement counts. UnflaggingEvents would still be fired. I'd rather remove the event later when we don't need it; Rules support is still developing.

Includes changes from 45 and 46. Does not re-add reset event.

Ok, triggering a reset should still be possible, especially if it's per node, where there are likely no scalability problems with huge amount of flags.

The reason I'm hesitant to just keep/readd the reset event is that it is broken for per-entity resets. As I wrote, in flag HEAD, if you trigger reset for a single node (I guess there is no way to do that except through the API), the reset event is triggered and FlagCountsManager deletes *all* counts for that flag.

Maybe that argument should simply be removed and you should use unflagAll() for that use case?

UnflagAllEvent? Split off into a separate issue?

The reason I'm hesitant to just keep/readd the reset event is that it is broken for per-entity resets.

I have to agree. This issue creates an UnflaggingEvent, which I think is a good improvement, but not as useful for Rules stuff. Here's one possible solution: #2713819: Remove reset(), augment unFlagAll().

Is there anything more to be done on this issue? It's holding up two other criticals.

Berdir says this is ready on IRC. ^_^

And committed! Thanks everyone! Please follow up in new or linked issues.