Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The vud_votingapi_results_alter function doesn't check if it is modifying its own results, so it modifies the results of every module that uses the Voting API.
Patch attached. Don't need if this patch is complete or good enough, but at least it solved our problems.
Comments
Comment #1
berenddeboer CreditAttribution: berenddeboer commentedComment #2
marvil07 CreditAttribution: marvil07 commentedAs this is about interoperability issue, I am changing priority.
Can you please some more details about the steps to reproduce your problem, so I would be able to review this?
Comment #3
berenddeboer CreditAttribution: berenddeboer commentedSteps? The hook implementation is clearly wrong. You alter the results for everything that ends up in your vud_votingapi_results_alter.
But install another module such as the Advanced Poll.. This also depends on the voting API. You'll see that your alter hook gets called when it shouldn't, and changes results when it shouldn't.
I.e. create a poll, after voting your alter hook is called and modifies the results. Eeks!
Comment #4
berenddeboer CreditAttribution: berenddeboer commentedComment #5
berenddeboer CreditAttribution: berenddeboer commentedComment #7
webindustries CreditAttribution: webindustries commentedany update to this? It would be awesome to get Fivestar and Vote Up/Down working together on the same site... I'm keen on commissioning a fix if this is required.
Comment #8
marvil07 CreditAttribution: marvil07 commentedberenddeboer: Yes, that's on propose, I mean, from vud module I am providing two new agreggation functions to votingapi module.
I do not see how this affects other modules that use voting_api, unless the other module is also implementing two aggregation functions with the same using the same names.
Can you please elaborate about it?
BTW the right way to filter is using the vote up down variable that store the tag name.
Comment #9
marvil07 CreditAttribution: marvil07 commenteda better title, actually
Comment #10
Agileware CreditAttribution: Agileware commentedThis is definitely a problem but I think it is a slightly different problem.
marvil07 is right that this is only supposed to be adding to the $cache, not modifying the existing $cache, but it is accidentally modifying the existing $cache also.
Seems to me the issue is this line in the vud_votingapi_results_alter() function:
$cache = array_merge_recursive($cache, $vud_cache);
If you check the value of $cache before and after this line you will see that array_merge_recursive() has reindexed the $cache array, meaning the tags have been modified, which is why everything is getting messed up.
So the tags in my $cache are being changed from 473, 474, 475, 476, 477 to 0, 1, 2, 3, 4 and then the advpoll module wants to match votes on their tags and can't because they've been changed.
I will try to investigate this further.
Comment #11
Agileware CreditAttribution: Agileware commentedChanging to needs work as I don't think the patch in the original post addresses the actual problem.
Comment #12
Agileware CreditAttribution: Agileware commentedHere is my solution.
Patch is against 6.x-2.x-dev but also applies cleanly to 6.x-2.1 (and quite possibly older versions too).
Basically, since vud only cares about the variable_get('vud_tag', 'vote') tag it has been changed to only merge that tag of the $cache array and leave the rest alone.
This approach (and any using array_merge_recursive()) could still cause problems if numeric array keys are used in the variable_get('vud_tag', 'vote') part of the $cache array. I don't have a deep understanding of this part of the $cache array so hopefully that won't be an issue.
Comment #13
Agileware CreditAttribution: Agileware commentedSince with this way you would not be merging the whole $cache array you could also remove the use of the
$tag = variable_get('vud_tag', 'vote');
variable from the _vud_get_standard_results() function.
Then you would end up with something like this.
- Patch reviewers note that this patch is mainly an option for the maintainer but is technically the same as the patch in #12.
So just review either of the patches.
Comment #14
marvil07 CreditAttribution: marvil07 commentedAgileware: thanks for the providing patches.
I think I could not find the problem you mentioned, but I find another really related one: the code was assuming there will be only one tag per content_type/content_id/vote_type, now I moved the logic again to the hook and since I am using a while, I also avoided calling array_merge_recursive, so I think this is solved.
It works fine for me, but I would really appreciate if someone can confirm this is working fine with advpoll before committing it.
Comment #16
Agileware CreditAttribution: Agileware commentedJust from my memory of the bug and looking at the patch I think that will solve the problem, because it shouldn't interfere with what is already in $cache.
The patch has some non-patch text in it though.
I'll test properly when I get back to work after the weekend though and confirm that it fixes the bug I have.
Comment #17
Agileware CreditAttribution: Agileware commentedI have tested your changes from #14 and it seems to work fine with my setup.
I think it is a better solution than the other patches too.
Here is your patch from #14 rerolled without the extra text.
Comment #19
marvil07 CreditAttribution: marvil07 commentedGreat, thanks for the review, committed to 3.x and 2.x.
BTW: bot is still failing on dependencies some times :-/, fails no related ;-)
Comment #20
Agileware CreditAttribution: Agileware commentedYeah, I had a quick look at the test fails and thought they were unrelated.