The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
The flag count service, in its getUserFlagFlaggingCount it does not perform any access checks. This results into having inconsistent flag counts in specific use cases.
The use case that I have is:
- Users needs to view the count of their favourite content.
- Currently, by using the getUserFlagFlaggingCount method, the count of favourite content is taking into account content which the user does not have access (either is unpublished, either the specific rules of the site do not allow them to access).
I am still not sure either this is a bug or it is a feature needed for the specific project, but it looks problematic for more use cases.
Steps to reproduce
Create a flag associated with a content type. Then create a node with a user without access to unpublished content and flag it. After that, unpublish it. The count of flags made by the user will take that node into account.
$flag = \Drupal::service('flag')->getFlagById('test_flag');
$current_user = \Drupal::currentUser();
$count = \Drupal::service('flag.count')->getUserFlagFlaggingCount($flag, $current_user);Proposed resolution
When calculating the user flagged counts, do not take into account the content that the user does not have access to.
Issue fork flag-3228503
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
omarlopesinoComment #4
omarlopesinoCreated a MR that check accesses to entities in order to get the count. Please review, thanks!
Comment #5
ivnishTests failed
Comment #6
omarlopesinoI've just adapted the tests to the new functionality, ready to review again.
Comment #7
ivnishComment #8
ivnishComment #9
ivnish@omarlopesino I don't see in the new test the case from steps to reproduce. Could you add it?
Comment #11
deaom commentedTried to reproduce this locally, was always getting the wrong count, meaning if user A flagged node, the count was 1. then user A lost access to the flagged node expecting the count to be 0, but it was always 1.
Added a test to see if it can be reproduced with test and it can be. The test should fail and it fails because when the getUserFlagFlaggingCount is first called it checks the access, which for now is allowed. Then the access is removed and the getUserFlagFlaggingCount called again, in this second call it does not check the access because the userFlagCounts gets populated and the else catches that as it's the same user and skips the access check part and just returns the count from before.
If in the test I remove the first call to the getUserFlagFlaggingCount, then the access check does not get skipped and "it works". But that is not how this should or would work. So code needs adjusting.
Also better steps to reproduce as this seems to be some custom code that explicitly calls the getUserFlagFlaggingCount.
Comment #12
deaom commentedAs the access check needs to be checked every time, the "cached" userFlagCounts is not needed here, as the information about access is missing, so removed that part of code. Test is now passing, but not sure this is the best approach for this. Like mentioned before, better steps to reproduce are needed to be more clear on what changes are actually needed. Changing status to needs review so somebody else can also have a look.
Comment #13
ivnishComment #14
ivnishI added a test code to steps to reproduce. I tested this MR manually, it works as expected
Comment #16
ivnish