Avoid fatal error when flagged entity is missing in flag_user_account_removal()

If the flaggable entity is missing, we should probably still remove the flagging.

Though then that would mean we'd have a call to flagging->delete() which bypasses the API, which I can't help but think sets a bad example :/

Tests added.

1. +++ b/src/Tests/FlagSimpleTest.php
   @@ -252,35 +252,45 @@ class FlagSimpleTest extends WebTestBase {
   +    $node1 = $this->drupalCreateNode(['type' => $this->nodeType]);
   +    $node1_id = $node1->id();
   +    $node2 = $this->drupalCreateNode(['type' => $this->nodeType]);
   +    $node2_id = $node2->id();
   ...
   +    $this->drupalGet('node/' . $node1_id);
   ...
   +    $this->drupalGet('node/' . $node2_id);
   ...
   +      ->condition('entity_id', $node1_id)
   ...
   +    $this->drupalPostForm('node/' . $node2_id . '/delete', [], t('Delete'));
   ...
   +      ->condition('entity_id', $node1_id)
   

   Just call $node1->id() or $node2->id().

2. +++ b/src/Tests/FlagSimpleTest.php
   @@ -252,35 +252,45 @@ class FlagSimpleTest extends WebTestBase {
   +    $this->clickLink('Flag this item');
   

   $this->clickLink(t('Flag this item'));

Done.

I think it's ok now, I made those changes.

I has asserting bad the tests, I have to compare no check if it's true, now the tests are ok.

+++ b/src/Tests/FlagSimpleTest.php
@@ -245,36 +247,45 @@ class FlagSimpleTest extends WebTestBase {
+    debug($count_flags_before);

One debug() left. :)

Yes, I forgot to remove, sorry.

+++ b/flag.module
@@ -275,13 +275,23 @@ function flag_user_account_removal(UserInterface $account) {
   $flaggings = $flag_service->getFlaggings(NULL, NULL, $account);
   foreach ($flaggings as $flagging_id => $flagging) {
-    $flag_service->unflag($flagging->getFlag(), $flagging->getFlaggable(), $account);
+    if ($flaggable = $flagging->getFlaggable()) {
+      $flag_service->unflag($flagging->getFlag(), $flaggable, $account);
+    }
+    else {
+      $flagging->delete();
+    }
   }
...
   $flaggings = $flag_service->getFlaggings(NULL, $account);
   foreach ($flaggings as $flagging_id => $flagging) {
-    $flag_service->unflag($flagging->getFlag(), $flagging->getFlaggable());
+    if ($flaggable = $flagging->getFlaggable()) {
+      $flag_service->unflag($flagging->getFlag(), $flaggable);
+    }
+    else {
+      $flagging->delete();
+    }

There is 96.86% similarity between these 2 blocks so I would suggest to create a helper method (in flag.module) and call it twice as it was before with unflag() method.

Won't change the status, as this is only a suggestion.

Here is your suggestion applied.

Looks nice. :)
Just small changes.

1. +++ b/flag.module
   @@ -12,7 +12,9 @@ define('FLAG_ADMIN_PATH_START', 3);
   +use Drupal\flag\FlagInterface;
   

   PhpStorm is saying this is not used.

2. +++ b/flag.module
   @@ -269,19 +271,31 @@ function flag_user_delete(UserInterface $account) {
   +function unflag(EntityInterface $entity = NULL, AccountInterface $account = NULL) {
   

   I would rename this to flag_unflag() (reads strange, but first word in the method name should be module name).

3. +++ b/flag.module
   @@ -269,19 +271,31 @@ function flag_user_delete(UserInterface $account) {
   +  unflag($account, NULL);
   

   And then change this flag_unflag($account). Second param is null by default.

4. +++ b/flag.module
   @@ -269,19 +271,31 @@ function flag_user_delete(UserInterface $account) {
   +    if ($flaggable = $flagging->getFlaggable()) {
   

   Possibly add /** @var \Drupal\flag\FlaggingInterface $flagging */ above this line.

Done all those changes.

1. +++ b/flag.module
   @@ -269,19 +268,32 @@ function flag_user_delete(UserInterface $account) {
   -  /** var \Drupal\flag\FlagServiceInterface $flag_service */
   -  $flag_service = \Drupal::service('flag');
   ...
   +  /** @var \Drupal\flag\FlaggingInterface $flagging */
   +  $flag_service = \Drupal::service('flag');
   

   I think this should be like before.

2. +++ b/flag.module
   @@ -269,19 +268,32 @@ function flag_user_delete(UserInterface $account) {
   +  $flaggings = $flag_service->getFlaggings(NULL, $entity, $account);
   

   And /** @var \Drupal\flag\FlaggingInterface $flagging */ one line above this one. :)

Other than that, looks good.

Done.

Looks nice. :)

Moved to service.

+++ b/src/Tests/FlagSimpleTest.php
@@ -245,36 +247,44 @@ class FlagSimpleTest extends WebTestBase {
+    // Ensure that are still flagged.

Que?

And:

You could also add a check to confirm that they haven't been deleted already before the user is deleted, but that would then fail in the follow-up that's suggested below. But that's ok too, file the issue and add a @todo to it.

Create the issue, add a @todo, update the issue summary (that one would be really helpful).

Moved the tests to a todo.

Create the issue, revert the removal of the test which will fail in the future, update the issue summary (that one would be really helpful).

+++ b/src/FlagService.php
@@ -266,6 +266,25 @@ class FlagService implements FlagServiceInterface {
+    $flag_service = \Drupal::service('flag');

We don't need to call service(), since this is now a just a FlagService method. We can just use $this. ^_^

I was wondering why we even need unflagAll() when we have reset(), and then I saw the different parameter lists. We should add some @see directives to FlagInterface::unflagAll() and FlagInterface::reset() to reduce confusion elsewhere.

Created the issue, added again the tests, added the correct @todo, removed service, added @see tags and edited the summary.

+++ b/src/Tests/FlagSimpleTest.php
@@ -272,7 +272,13 @@ class FlagSimpleTest extends WebTestBase {
+    // Ensure that are still flagged.

This comment still doesn't make any sense.

Done, I hope.

Better.

Hooray! Looks good to go.

Rebased.

Wrong file.

Rebase Needed.

Rebased.

Some nitpicking and good to go

1. +++ b/flag.module
   @@ -266,20 +264,12 @@ function flag_user_delete(UserInterface $account) {
   -
   

   Unrelated empty line

2. +++ b/src/Tests/FlagSimpleTest.php
   @@ -245,36 +247,45 @@ class FlagSimpleTest extends WebTestBase {
   +    // Flag both nodes.
   

   improve the comment.

Done.

Unrelated failing test.

Okay tests are good, setting to RTBC.

Sorry I've not given this any attention. I've been aware of people working on this one.

I think unflagAll() is a good addition. Though reading this patch, I was thinking about how (IIRC) we don't yet take care of deleting flaggings when a flag is deleted. Which let me to thinking that when we tackle that, unflagAll() would acquire an extra $flag parameter. Which then makes reset() a bit pointless really -- reset() is just a special case of unflagAll() where we only specify the $flag parameter. The 'reset' part becomes legacy terminology from previous version of Flag. But that can be dealt with in a clean-up issue later.

The patch does have a few things that need fixing:

1. +++ b/flag.module
   @@ -9,13 +9,11 @@ define('FLAG_API_VERSION', 3);
   -use Drupal\flag\Entity\Flag;
    use Drupal\node\NodeInterface;
    use Drupal\Core\Entity\EntityInterface;
    use Drupal\Core\Entity\Display\EntityViewDisplayInterface;
    use Drupal\Core\Routing\RouteMatchInterface;
    use Drupal\user\UserInterface;
   -use Drupal\Core\Form\FormStateInterface;
   

   That looks like unrelated cleanup.

2. +++ b/flag.module
   @@ -266,20 +264,14 @@ function flag_user_delete(UserInterface $account) {
   -  /** var \Drupal\flag\FlagServiceInterface $flag_service */
   +  /** @var \Drupal\flag\FlagService $flag_service */
   

   This shouldn't be changed. The @var type declaration is an interface. See standard proposal: https://www.drupal.org/node/2305593.

3. +++ b/src/FlagService.php
   @@ -268,6 +268,25 @@ class FlagService implements FlagServiceInterface {
   +    /** @var \Drupal\flag\FlagServiceInterface $flag_service */
   +    $flag_service = \Drupal::service('flag');
   

   We're in the service here! It's $this ;)

4. +++ b/src/FlagService.php
   @@ -268,6 +268,25 @@ class FlagService implements FlagServiceInterface {
   +      if ($flaggable = $flagging->getFlaggable()) {
   +        $flag_service->unflag($flagging->getFlag(), $flaggable, $account);
   +      }
   +      else {
   +        $flagging->delete();
   +      }
   

   This needs a comment to explain that we're working around the potential situation where the flagging is already gone. Also, it should be mentioned that the unflag event won't get fired. Basically, if we get here, it's because our data is broken and we're trying to do our best to clean it up.

5. +++ b/src/FlagServiceInterface.php
   @@ -148,16 +148,31 @@ interface FlagServiceInterface {
   +   *    Will remove all flags done to this entity.
   ...
   +   *    Will remove all flags by this account.
   

   Say 'flaggings', not 'flags' here.

6. +++ b/src/FlagServiceInterface.php
   @@ -148,16 +148,31 @@ interface FlagServiceInterface {
   +   * @see FlagServiceInterface::remove()
   +   *    Not be confused with remove().
   +   */
   

   remove()?

7. +++ b/src/Tests/FlagSimpleTest.php
   +++ b/src/Tests/FlagSimpleTest.php
   @@ -7,9 +7,11 @@
   

   Hmmmmyeah this test class is a bit of a dumping ground for random bits. I really do appreciate the tests though :)

8. +++ b/src/Tests/FlagSimpleTest.php
   @@ -245,36 +247,45 @@ class FlagSimpleTest extends WebTestBase {
   +    // Add flags to the nodes.
   

   'Flag the nodes.'

9. +++ b/src/Tests/FlagSimpleTest.php
   @@ -352,4 +363,27 @@ class FlagSimpleTest extends WebTestBase {
   +  /**
   +   * Count the number of flags of the user over an entity.
   +   *
   

   Is there a reason we're not using the count service? I suppose tests should query the DB directly. Should this go in the base test class maybe?

10. +++ b/src/Tests/FlagSimpleTest.php
    @@ -352,4 +363,27 @@ class FlagSimpleTest extends WebTestBase {
    +    // Ensure there is still one flagged.
    

    Comment doesn't fit here.

Done everything.
In the point 9, I don't have any method in count service to count the flags of a user over an entity, if we have to add it there, then maybe it should be done in a follow up?
The other solution of moving it to base test, means that FlagSimpleTest has to extend base test, or maybe as now most of the things we are testing are in services maybe we should move the tests to that other test class?

Verified everything appears to be fixed.

In the point 9, I don't have any method in count service to count the flags of a user over an entity, if we have to add it there, then maybe it should be done in a follow up?

We should create a follow up and add it to the count service if it sounds like it has value. That can be done in another issue.

Argh, needs a reroll, sorry. My fault for taking too long to get round to committing this.

Rebased.

Yup, I agree with all those points.

> I don't think @see supports a description? What about making this an inline documentation that references that. Combined with the above.

Yup. Also, don't say reset() as that's a core PHP function!

> * I actually find "Unflag all flags" quite confusing, the flagging vs flag terminology is tricky. reset() uses "Remove all flagged entities", which is better, but what we actually remove is the flagging, not the entity, so that's kind of confusing too. "Removes all flaggings" might be simpler and clearer? Then we could unify those two descriptions.

Yup. "Remove all flaggings made with this flag / on this entity" is the clearest.

Rebased and applied those changes.
I deleted the @see as FlagServiceInterface doesn't define the reset() anymore.

Everything looks good except this comment is missing:

Also, what happens if neither is provided? Delete everything? Is that supported? [...] Should probably be mentioned.

Added this comments and changed the short description to "Remove all flaggings from an entity / made by an account."
I think has more sense as this method never removes for a flag, always remove on all the flags just depending on the entity or account.

I think the comments should be correct and explanatory now, good to go!

Sorry to knock this back another time :(

There's a few things in unflagAll() that don't quite make sense to me.

1. +++ b/flag.module
   @@ -7,7 +7,6 @@
   -use Drupal\Core\Access\AccessResult;
   

   Leave this in please -- it's there for commented-out code.

2. +++ b/src/FlagService.php
   @@ -272,6 +272,25 @@ class FlagService implements FlagServiceInterface {
   +  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 ($flaggable = $flagging->getFlaggable()) {
   

   If $entity is specified, the it's the $flaggable, isn't it? So you don't need to keep loading it.

3. +++ b/src/FlagService.php
   @@ -272,6 +272,25 @@ class FlagService implements FlagServiceInterface {
   +        $this->unflag($flagging->getFlag(), $flaggable, $account);
   

   ... and if $account wasn't given, you're passing a NULL in here!

4. +++ b/src/Tests/FlagSimpleTest.php
   @@ -235,36 +237,48 @@ class FlagSimpleTest extends FlagTestBase {
        $node = $this->drupalCreateNode(['type' => $this->nodeType]);
        $node_id = $node->id();
   +    $node2 = $this->drupalCreateNode(['type' => $this->nodeType]);
   

   While we're rerolling... could this be $node1 and $node2? Or $node and $node_to_delete?

   And also $node_id is a bug waiting to happen now :/

5. +++ b/src/Tests/FlagSimpleTest.php
   @@ -356,4 +370,26 @@ class FlagSimpleTest extends FlagTestBase {
   +   * Count the number of flags of the user over an entity.
   

   This isn't quite right -- it counts flaggings, not flags.

6. +++ b/src/Tests/FlagSimpleTest.php
   @@ -356,4 +370,26 @@ class FlagSimpleTest extends FlagTestBase {
   +  protected function countFlagging(UserInterface $user, NodeInterface $node) {
   

   countFlaggings? or getFlaggingCount?

Done all those points, also deleted flag count service that I'm loading and not using, I think was result of one rebase.

I think we can refactor flag_user_account_removal() some more- It is the procedural way of sharing common functionality between hooks.
I have moved the logic into the FlagService::userFlagRemoval()

There is still work todo ... so I am just making this change in isolation ... I want to keep the interdiff simple and easy to follow.

I have had a little review and this look like a good change.

It is out of scope but I am going to mention it here just in case it raises things for others.

I was manually going through the steps to both cancel and delete looking to see if anything caused problems

Using /admin/people to delete a account gives these options.

Disable the account and keep its content.
Disable the account and unpublish its content.
Delete the account and make its content belong to the Anonymous user.
Delete the account and its content.

We need a way for the admin to choose to respond in ways that don't delete in every case.
There is nothing wrong with anonymous users flagging content - I will file an issue for that.

Nuts ....so much for simple and easy to follow :(

Sorry for add the interdiff with the patch 62, but I could not apply the patch 64 so I added those changes manually.
I fixed the comment 63.

Sorry for deliting your file from the file list, but I didn't want to do that, drupal.org did that when I Saved my comment, and your post was already posted :( And I don't have any option tho make it appear again, I don't like how it works when more than one are publishing at the same time.

And we discused with @Berdir that we should create a followup to remove the flags in predelete instead of in delete, so we have to implement flag_user_predelete

The merging of patches looks good to me.

I can see another problem .. with FlagService not the last patch

I am going to create a new side issue but I should talk about it here because if we cannot resolve this independently then it makes the approach taken here bad.

The issue is between

\Drupal\user\UserInterface
\Drupal\Core\Session\AccountInterface;

AccountInterface is the larger superset containing all of UserInterface.

The patch introduces UserInterface as an ugly ducking method parameter to FlagService - all other methods use AccountInterface.

Regarding the flow of a UserInterface variable flowing into the service

flag_user_delete() -> FlagService::userFlagRemoval() -> FlagService::unFlagAll() -> FlagService::getFlagging/FlagService::unflag()

So I think we need to make some flag and flagging calls work with the more limited UserInterface.

Anyway I will create the issue tomorrow.

Hangs head in shame.... to make up for my mistake.

Here is a fix to ugly duckling problem ...now all FlagServices use the AccountInterface.

Then, no need to set it to needs work, the previous patch was just fine.

Not related with this issue:
I know why my comment deleted your file, because both of the patches had the same name, This is a bug of drupal.org site, not sure where to report it, the files maybe need some kind of unic id.

#67 no longer applies

Since the conflicts were only comment changes from another issue, there's no need to re-review. Committing.

Thanks everyone!