advpoll_node_delete() can inadvertently delete non-node votes because it deletes votes based solely on entity_id. It should delete on entity_id AND entity_type because an entity_id is unique to an entity_type, but there could be entities of different types with the same IDs.

CommentFileSizeAuthor
#4 recent-block-error-and-table-prefix-brackets-1559616.patch6.51 KBMichele Wickham
#2 vote-deletion-1555066-2.patch10.38 KBMichele Wickham
#1 vote-deletion-1555066-1.patch2.21 KBMichele Wickham

Comments

Michele Wickham’s picture

Michele Wickham commented
Assigned: Unassigned » Michele Wickham
StatusFileSize
new vote-deletion-1555066-1.patch2.21 KB

I've attached a patch that includes an additional condition to select only votes with entity_type advpoll.

Michele Wickham’s picture

Michele Wickham commented
StatusFileSize
new vote-deletion-1555066-2.patch10.38 KB

Use this patch please. I posted the wrong one.

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented

I'm not going to have a chance to test the patch (I was just browsing the code evaluating whether this module meets my needs), but the logic looks correct.

A personal stylistic note: I prefer chaining database functions, eg:

db_delete('mytable')
  ->condition('nid', 4)
  ->condition('status', 1)
  ->execute();

I find it easier to read and I believe there can be a (tiny) performance advantage.

Michele Wickham’s picture

Michele Wickham commented
Status: Active » Needs review
StatusFileSize
new recent-block-error-and-table-prefix-brackets-1559616.patch6.51 KB

I've created a patch that incorporates a minor tweak to the query. It turns out that you need to call a db_and() when you need both conditions to be true on a query. The above example would read as an OR - so if either condition is true then the row gets deleted. The latest version will include a structure more like this:

$and = db_and()->condition('entity_id', $node->nid)->condition('entity_type', 'advpoll');
db_delete('votingapi_vote')->condition($and)->execute();

rather than how I had it before with just a single condition. The patch applies to the version that is currently available on the project page (although within 24-hours it should be fully incorporated into the build).

Thanks!
Michele

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Status: Needs review » Needs work

You don't need to call db_and(); it's unnecessarily complex. Per http://drupal.org/node/310086,

By default, that conjunction is AND

You can do this:

db_delete('votingapi_vote')
  ->condition('entity_id', $node->nid)
  ->condition('entity_type', 'advpoll')
  ->execute();
Michele Wickham’s picture

Michele Wickham commented

I'll look at it again - it seemed when I tested the query without the db_and it was deleting too many rows during node_delete.

Michele Wickham’s picture

Michele Wickham commented
Status: Needs work » Fixed

Have not heard back since I made the update to address this issue. Closing the bug.

Michele Wickham’s picture

Michele Wickham commented
Status: Fixed » Closed (fixed)
jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Status: Closed (fixed) » Active

I recommend not passing on this bug, which is relatively severe.

jacob.embree’s picture

jacob.embree commented
Assigned: Michele Wickham » Unassigned
Issue summary: View changes
Status: Active » Fixed

This actually is fixed by part of #2. The relevant portions were committed.

Status: Fixed » Closed (fixed)

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