Voting Storages does not check access on entity query

Added a MR please review.

This was done long ago for VoteStorage, see #3267731: [9.2] Access checking must be explicitly specified on content entity queries → , but it seems like VoteResultStorage was not touched at that time.

Because our automated testing works and runs without error on Drupal 9 and Drupal 10, I'm curious how this error gets triggered. At a minimum, we need a new test that uses VoteResultStorage - that would have caught something like this long ago. Can you provide directions for reproducing this problem, and add a test to the patch?

This also has to be fixed in VotingApiCommands, but since we don't make it a practice of writing tests for Drush commands that should be easy.

The issue is not coming up when this module is used on its own. I get the error when using the module https://www.drupal.org/project/like_and_dislike → which uses votingapi as the dependent module.

The issue is reproducible when we attach a like and dislike widget on any entity display.
The like and dislike widget makes a call to VoteResultsStorage to get the counts for the likes and dislikes. Here is the trace:

Drupal\Core\Entity\Query\Sql\Query->prepare() (Line: 80)
Drupal\Core\Entity\Query\Sql\Query->execute() (Line: 29)
Drupal\votingapi\VoteResultStorage->getEntityResults('node', '1', 'like', 'vote_sum') (Line: 65)
like_and_dislike_get_votes(Object) (Line: 75)
Drupal\like_and_dislike\LikeDislikeVoteBuilder->build('node', '1')

I have noticed that the votingapi module is not using the VoteResultsStorage anywhere to get the counts. Instead there is custom plugin manager service defined VoteResultFunctionManager which is used instead.
There is a functional test written for the same.

What would be the recommend approach moving forward considering VoteResultsStorage might be used by other modules. I can provide the patch accordingly.

I don't know what the thoughts were at the time, since I was not involved in that.

Personally, I see the storage for an entity as something that is part of the implementation of the entity, not part of the API. I don't see a need to have a customized storage class VoteResultStorage just to add one convenience method which doesn't even require knowledge of the underlying storage (because all it is doing is an entityQuery()). If the method relied on explicit SQL calls, then I could understand putting the method in a subclass of SqlContentEntityStorage, but it doesn't. Apparently Drupal core uses the same pattern of putting entity query methods into custom storage classes, so I can't blame people for copying that, but it just seems wrong to me.

I think a service is a far better place to put methods that get the vote results. But that's not something we can really change without a new major version branch, because we would have to deprecate the old methods and remove the custom storage. I would recommend using the service to get the results, and adding methods to the service if it doesn't have all the functionality it needs.

For now I think we first have to make sure VoteResultStorage works, and your patch is part of that. Adding a test for VoteResultStorage::getEntityResults() is the second part. Then when we do get around to refactoring this we will at least have a test to make sure any new code works the same as the old code.

I was going to recommend a core test as a template for writing the test, but to my dismay core doesn't seem to have any tests for any of its customized storage classes? I may have missed some, but all the core modules I looked at didn't test their storage. For example look at NodeStorageInterface, which defines 5 custom methods. Three of those methods are not used anywhere in Drupal core at all, not even in the node module, and two of those methods are used only once or twice in the entire core. And NONE of those custom methods have tests to ensure they do the right thing.

So I don't know what to tell you. My preference is just to add the accessCheck() and also add a simple test for getEntityResults(). The test will demonstrate the current failure and prove that your patch fixes that failure.

Added the requested changes in the MR.
Please review.

In #3267731: [9.2] Access checking must be explicitly specified on content entity queries → , it was missed that getVotesSinceMoment() needs to add accessCheck(). I'm going to include that here and make this a more generic access check issue.

I did a quick scan of the code to see if there are any other missing access checks. I didn't find any.

Desperately needed for D10 compatibility

10.1.0 is released this week
10.0.0 was released last year

This patch works with Drupal 10.0.9, thank you!

Attaching a static patch of the current MR for deployment purposes.

It is not necessary to add a patch file when issues have been resolved via Merge Request, you can follow this guide https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-drupal/downloading-a-patch-file → , then add the patch from local file in the composer.json file.

Merged. Thanks.

Automatically closed - issue fixed for 2 weeks with no activity.

A new release would be nice if the module if flagged as Drupal 10 compatible. Now updating to latest version of this module is not enough, you need to look in the issue queue and go through closed issues to get a patch to get the module to work.

Could we release a new tag of this module so that D10 support is actually possible? Its been 7 months since a release and 2 months since this was committed. Without this patch, the module fails miserably on D10.

What is preventing these changes from being released?.

Today I updated my blog site with Drupal 10.3 which uses like_and_dislike → module that depends on this votingapi module, and when I visited one of my page, I can see WSOD

I had to use this MR's changes to patch the module, which fixes the issue.

"patches": {
            "drupal/votingapi": {
                "3341513 - Voting Storages does not check access on entity query": "https://git.drupalcode.org/project/votingapi/-/merge_requests/5.diff"
            }
        }