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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

berenddeboer’s picture

Priority: Normal » Critical
marvil07’s picture

Title: Module doesn't play nice with other voting modules installed due to bug in vud_votingapi_results_alter » Module doesn't play nice with other voting modules installed
Priority: Critical » Normal
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +interoperability

As 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?

berenddeboer’s picture

Steps? 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!

berenddeboer’s picture

Status: Postponed (maintainer needs more info) » Needs review
berenddeboer’s picture

Priority: Normal » Critical

Status: Needs review » Needs work

The last submitted patch, vud.module.patch, failed testing.

webindustries’s picture

any 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.

marvil07’s picture

Title: Module doesn't play nice with other voting modules installed » Calculate Module doesn't play nice with other voting modules installed
Priority: Critical » Normal
Status: Needs work » Postponed (maintainer needs more info)

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.

berenddeboer: 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.

marvil07’s picture

Title: Calculate Module doesn't play nice with other voting modules installed » Aggregation functions must be only for vud tag votes?

a better title, actually

Agileware’s picture

Title: Aggregation functions must be only for vud tag votes? » array_merge_recursive re-indexing $cache array, breaking other votingapi modules
Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Active

This 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.

Agileware’s picture

Status: Active » Needs work

Changing to needs work as I don't think the patch in the original post addresses the actual problem.

Agileware’s picture

Version: 6.x-2.0 » 6.x-2.x-dev
Status: Needs work » Needs review
FileSize
885 bytes

Here 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.

Agileware’s picture

Since 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.

marvil07’s picture

Agileware: 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.

Status: Needs review » Needs work
Agileware’s picture

Just 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.

Agileware’s picture

Status: Needs work » Needs review
FileSize
2.63 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, vote_up_down-array_merge_recurive-890404-17.patch, failed testing.

marvil07’s picture

Status: Needs work » Fixed

Great, thanks for the review, committed to 3.x and 2.x.

BTW: bot is still failing on dependencies some times :-/, fails no related ;-)

Agileware’s picture

Yeah, I had a quick look at the test fails and thought they were unrelated.

Status: Fixed » Closed (fixed)
Issue tags: -interoperability

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