FlaggingStorage does not update cached Flagging status.

Not bad so far! Needs tests.

This also needs to implement the resetCache() method to clear these variables, something like:

  /**
   * {@inheritdoc}
   */
  public function resetCache(array $ids = NULL) {
    parent::resetCache($ids);
    $this->flagIdsByEntity = [];
    $this->globalFlagIdsByEntity = [];
  }

but it would ideally take into account the $ids variable and selectively unset them instead.

This adds a test to demonstrate the issue, and adds the cache reset method.

This patch actually resolves #2826227: Test to verify flagging events with multiple flags. I've re-rolled that issue, as it is probably still worth committing the functional javascript test once this issue is committed.

Re-rolled to use the flag create trait now that that is in.

I like this issue a lot, the description of the bug is excellent, and the solution looks good.

Regarding the test

This looks good to me,

it uses nothing but standard techniques
it exposes the problem.

the patch as a whole is great. so +1 from me.

My patch fixes a minor nit only.

FlaggingStorageTest::flaggingStorage is defined but unused so I have removed it.

Additionally, I have an alternate perspective which I am not minded to do anything about!

Just for discussion FlaggingStorageTest exercises only FlagService methods to get at the underlying Storage...
I would have argued that at this level we don't care about the underlying storage implementation, or any future implementation we test , we only care that isFlagged() always responds correctly.

In short, I would have been added a test method to the FlagServiceTest without creating a whole new test class.
but I don't want to step in the way or Jhedstrom's design choices, which are all perfectly reasonable.

In short, I would have been added a test method to the FlagServiceTest without creating a whole new test class.

I like that idea myself, but when I start looking at the code it doesn't seem like the right way forward. This is testing the cache reset of FlaggingStorage. Which leads me to the next problem:

+++ b/src/Entity/Storage/FlaggingStorage.php
@@ -27,6 +27,15 @@ class FlaggingStorage extends SqlContentEntityStorage implements FlaggingStorage
+  public function resetCache(array $ids = NULL) {
+    parent::resetCache($ids);
+    $this->flagIdsByEntity = [];
+    $this->globalFlagIdsByEntity = [];
+  }

If this is public, shouldn't it be a part of FlaggingStorageInterface? Then a separate test makes a lot more sense.

If this is public, shouldn't it be a part of FlaggingStorageInterface? Then a separate test makes a lot more sense.

The resetCache method is part of EntityStorageInterface, which this one extends, so no need to redeclare that method on the flagging storage interface.

As for the separate test method, kernel tests are very lightweight compared to web/browser tests, so adding a storage specific test class doesn't add much in terms of run time. It also is specific to the storage class, rather than the flag service. However, I don't want to block this fix, so either way is fine by me :)

Setting back to needs review for #13.

The resetCache method is part of EntityStorageInterface, which this one extends, so no need to redeclare that method on the flagging storage interface.

Makes sense. I can't find anything else to hold up this patch.

Thanks!