Since just adding a fivestar field to one of my cck-types brought several errors on saving a node, I've gone a bit over the module.
With the applied patch the values are now saved correctly in the cck-table, no float-left leftovers after the stars, setting $prev_star_value so we can use it for displaying partial stars and start of support to not only vote on a node, but also on a tag on a node.

Comments?

CommentFileSizeAuthor
#6 fivestar_node_load.patch1.21 KBquicksketch
#4 fivestar-var-fixes-2.patch2.04 KBray007
fivestar-var-fixes-1.patch6.86 KBray007

Comments

ICDeadPpl’s picture

ICDeadPpl commented

How do I apply this patch?

ray007’s picture

ray007 commented

see http://drupal.org/patch/apply

quicksketch’s picture

quicksketch
he/him
commented

Could you split these up? I'm not sure about all the changes. Some things are bug fixes while others are tasks or features.

ray007’s picture

ray007 commented
StatusFileSize
new fivestar-var-fixes-2.patch2.04 KB

All right, attached here is the fixes-only part.
The other part, as you saw, were first preparations to allow fivestar to rate tags instead of a whole node (or other content object).

One thing to note: as it is the fivestar cck-field won't be able to actually collect votes on anything that hasn't already fivestar votes enabled, because the check in _fivestar_validate_target() will always return false. There should probably be an override flag as parameter to _fivestar_cast_vote() to skip that check ... but I guess that's bringing us to "new feature" territory again ...

Just tell me when you want the changes for adding $tag in various places ...

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Needs work

I committed your changes with one exception:

-        $target_node = node_load($items[$delta]['target']);
+        if ($items[$delta]['target']) {
+          $target_node = node_load($items[$delta]['target']);
+        }
+        else {
+          $target_node =& $node;
+        }

This is a little bit odd (and I'm not really sure it's a fix). Hypothetically: If there isn't a target node, register a vote on this node? I think that if a target node doesn't exist, no vote should be registered at all. The "vote" would simply exist as an integer stored in the cck field. However, there probably should be a check to see if $items[$delta]['target'] is set before trying to load a node on it. What do you think?

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Fixed
StatusFileSize
new fivestar_node_load.patch1.21 KB

Okay, *some* fix was applied for every issue in the second patch. Thank you for pointing out all these separate problems! Next time, please update existing tickets or create new ones for each issue. I know it's easy to get carried away and do it all in one patch :)

Thanks again!

(btw, patch attached for what I used for the last fix. Just a simple check instead of voting on that node.)

ray007’s picture

ray007 commented

When installing fivestar and using the cck-field without the targetnode fix I got lots of nasty error messages.
Writing the value to the cck-field is exactly what happens in this case, since the target-validation fails in _fivestar_cast_vote().

I still think we should set the target-node to self and have another property on the field which tells us, if it's a field where just the owner/editor sets a value, or where the viewers can vote. This becomes especially interesting if you look at the rest of the first patch, which was on the way to allow voting on tags ...

ray007’s picture

ray007 commented

IIRC setting the style clear on a BR-tag doesn't work in all browsers. That's why I added an empty DIV ...

quicksketch’s picture

quicksketch
he/him
commented

The patch above fixes those nasty SQL errors (and has been committed).

It seems as though if the user needed to record a vote on the node itself, they could just use the php code:

return $node->nid;

If no PHP code is entered, I would assume it is because they don't actually want the vote to be registered.

quicksketch’s picture

quicksketch
he/him
commented

IIRC setting the style clear on a BR-tag doesn't work in all browsers. That's why I added an empty DIV ...

Yes I know, I used the "clear-block" class provided by Drupal 5, which properly clears in all browsers. I didn't use the empty div because it's invalid markup.

killes@www.drop.org’s picture

killes@www.drop.org commented
Status: Fixed » Closed (fixed)