isFlagged status is not reflected correctly in some cases, most cases of this will incorrectly show the wrong flag status until the page is refreshed.
Two calls to $flag->isFlagged($entity) that span a Flagging save or a Flagging delete operation will not update the isFlagged status.
Ex.
$flag->isFlagged($entity); (returns Not Flagged)
Call to FlagService flag with $entity.
$flag->isFlagged($entity); (returns Not Flagged Incorrectly)
OR
$flag->isFlagged($entity); (returns Flagged)
Delete Flagging for $entity for $flag_id
$flag->isFlagged($entity); (returns Flagged Incorrectly)
Since the addition of the FlaggingStorage (https://www.drupal.org/commitlog/commit/6408/cf93be36cd4f89a0f6ddafe0bbb...) this has been an issue.
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff-10-11.txt | 1.01 KB | martin107 |
#11 | 2801423-11.patch | 6.12 KB | martin107 |
#6 | 2801423-06.patch | 6.38 KB | jhedstrom |
#6 | 2801423-06-TEST-ONLY.patch | 3.23 KB | jhedstrom |
Comments
Comment #2
rbrandon CreditAttribution: rbrandon commentedI have tested a fix for this that updates the static cache in flaggingStorage.
Comment #3
rbrandon CreditAttribution: rbrandon commentedComment #4
socketwench CreditAttribution: socketwench as a volunteer commentedNot bad so far! Needs tests.
Comment #5
jhedstromThis also needs to implement the
resetCache()
method to clear these variables, something like:but it would ideally take into account the
$ids
variable and selectively unset them instead.Comment #6
jhedstromThis adds a test to demonstrate the issue, and adds the cache reset method.
Comment #9
jhedstromThis 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.
Comment #10
jhedstromRe-rolled to use the flag create trait now that that is in.
Comment #11
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #12
socketwench CreditAttribution: socketwench as a volunteer commentedI 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:
If this is public, shouldn't it be a part of FlaggingStorageInterface? Then a separate test makes a lot more sense.
Comment #13
jhedstromThe
resetCache
method is part ofEntityStorageInterface
, 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 :)
Comment #14
jhedstromSetting back to needs review for #13.
Comment #15
socketwench CreditAttribution: socketwench as a volunteer commentedMakes sense. I can't find anything else to hold up this patch.
Comment #17
socketwench CreditAttribution: socketwench as a volunteer commentedThanks!