Posted by eaton on July 1, 2009 at 7:46am
| Project: | Plus 1 |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | D7 porting |
Issue Summary
Testing out the D7 port of VotingAPI -- needed a voting module to do the testing with and thought I'd get this one ported. Here's a (very rough) version.
| Attachment | Size |
|---|---|
| plus1-d7.patch | 4.19 KB |
Comments
#1
Excellent! Subscribing to this one to make sure that I look further at it when I get time.
#2
Any chance we can get a commit and dev snapshot for plus1 d7? would help with my testing of module dependencies
#3
@Moshe: Wouldn't it be good if someone took time to test the patch prior to committing it?
#4
The older patch was out of date. The attached one accounts for changes in the D7 theming APIs, and two property name changes in the VotingAPI 7.x release.
There's a problem with the jQuery code that I haven't been able to figure out: when clicked, widgets cast votes but the on-screen widget does NOT change, and the page must be reloaded to reflect the new vote total.
#5
And here's the latest version -- the JS works, and everything seems to be humming along smoothly.
#6
I second that.
#7
Tag
#8
subscribe
#9
Jeff, while I have no problems with patching, there are many who do. Perhaps for them you could attach a complete tarball or zip file?
#10
My name ain't Jeff, but I got the port up and running on D7 final using Vote-api 7.x-2.x-dev
Also moved the theme output into a separete template, because I do believe that is easier for themers to work with ;)
Todo is add an option to promote the widget into a block, so I could use it using a context alike module.
My work is based on the patch of Jeff, and added a few bit here and there :)
Code is on gitorious : http://gitorious.org/plus1-d7/plus1-d7/trees/master (or use for a tarball
#11
Awesome!
#12
@ReneB Post #10.
I believe I've found a bug in your code (or votingapi, to be honest, I'm not sure).
Problem is with plus1's views integration and I've tracked the code down to plus1 problem or votingapi problem. I think it's a plus1 problem.
Currently plus1 is saving values into the database's votingapi_vote table as "entity_type = $node->type". I think it's should simply be "node" or "comment". votingapi when it's doing it's views integration doesn't look for specific node types. Nor to I think it should be. I think it only make the distiction between "comments" and "nodes" and potentially other that modules can implement themselves.
With that said, it's an easy fix to you code. Simply change
diff --git a/plus1.module b/plus1.moduleindex 88c00a1..48e2b62 100644
--- a/plus1.module
+++ b/plus1.module
@@ -145,7 +145,7 @@ function plus1_vote($nid) {
$node_type = db_select('node','n')->fields('n', array('type'))->condition('nid', $nid)->execute()->fetchObje
$votes[] = array(
'entity_id' => $nid,
- 'entity_type' => $node_type->type,
+ 'entity_type' => 'node',
'value_type' => 'points',
'value' => 1,
);
I've attached this diff and might commit to glitorus after more testing (if I have access to do that)
#13
I created my own gitorious repo, but I have no idea how to request a merge. I removed another line which has no need any more.
--- a/plus1.module+++ b/plus1.module
@@ -142,10 +142,9 @@ function plus1_vote($nid) {
$voted = plus1_get_votes($nid, $user->uid);
// If the voter has not already voted.
if (!$voted) {
- $node_type = db_select('node','n')->fields('n', array('type'))->condition('nid', $nid)->execute()->fetchObje
ct();
$votes[] = array(
'entity_id' => $nid,
- 'entity_type' => $node_type->type,
+ 'entity_type' => 'node',
'value_type' => 'points',
'value' => 1,
);
#14
@ReneB Post #10:
I just found another minor issue. Following code can be found twice in "plus1_settings()" and can be removed once:
$form['plus1_display']['plus1_in_full_view'] = array('#type' => 'checkbox',
'#title' => t('Add a +1 voting widget to the node in full view.'),
'#default_value' => variable_get('plus1_in_full_view', 1),
);
#15
I removed hook_init() function call, in favour of a static variable. hook_init() get's called on every page load and is not really needed for what the initial programmer is trying to accomplish. Static variable I believe should do this fine, since it's life span is a page request.
Code is on my gitorious.
I'm planning on extending this module to allow for multiple votes on the same node, after a function call is made. I believe I'll make that function call available to the triggers module and/or cron.
I need this for something I'm doing. If the project doesn't want to include it, I'll release a splinter module called like PlusN or something :) Which will be a simple fork with some mild additional logic / admin settings added.
#16
Trigger/Action: This should be in VotingApi and there is at least one issue related to that (although Rules, I think).
Removing hook_init: see #696500: Variable misused used to store state which will be committed to the 6.x branch shortly.
For all interested, please check the list of "Patch to be ported" as these will all be committed on the 6.x branch as soon as we get the permission.
#17
@eaton: Jeff thanks for this work. Would it be possible to re-roll with all the latest fixes?
#18
#19
When we get dev version for D7? There is even no CVS branch for it in the CVS atm.
#20
@OnkelTem: See #17 - there needs to be code to commit prior to a commit being possible :)
#21
Yeah i noticed that to, fixed it but no time to commit it yet... But the issue was with votingAPI i guess or with the -dev version of views and votingAPI I'm using cause for some reason I can not select votes in a view.
Thanks for forking/fixing :)
#22
@alpileinYeah i noticed that to, fixed it but no time to commit it yet... But the issue was with votingAPI i guess or with the -dev version of views and votingAPI I'm using cause for some reason I can not select votes in a view.
@j0rd Thanks for forking/fixing :)
@onkelTem Pull the code from one of the two above mentioned gitorious accounts i you need this module, since those are working too, or do you need code to be 'officially' released trough d.o system?
@nancyDru, how about just checking in the work already done by us into a 7-alpha/beta version? Would make live a little easier for patches to be centralized.
#23
subscribing
#24
@NancyDru can you commit current work into 7-dev, it make much easy to polish code.
#25
Subscribing
#26
+1 :)
#27
I am still working on getting set back up after I had my hard disk rebuilt, so I don't have a D7 operating yet.
#28
Subscribing
#29
Having a little oddness here. I downloaded the tarball in post #10, then applied the changes described in posts #12, #13, and #14.
It now appears that a user can vote, and vote only once, and I can sort by score in views. However, the score does not actually display on the vote widget; it displays 0 if no votes have been received, and 1 if any number of votes have been received. Any suggestions appreciated.
#30
I'm looking forward to a dev release. That is music to my ears.
#31
No -dev release while this is marked "needs work."
#32
Hello all!
I needed this module in a one Drupal 7 project. So i have ported it and willing to share my work. I uploaded the module completely instead of patch, because lots of things was rewritten. Here is the list of features:
theme_plus1_widget — used to theme widget itself
theme_plus1_json_response — used to theme ajax response to your widget. this gives you a chance to provide your javascript with any kind of data, like rendered widget, or only the score, or may be some message. By default, module reloads the widget, when voting is occurred.
Theming functions support template suggestions like plus1_widget__ENTITY_TYPE__TAG or plus1_json_response__ENTITY_TYPE__TAG
Hope you'll find it useful. And if module owner will consider these changes valuable, i would love to become a co-maintainer and commit it.
Cheers
#33
Whew, that's a lot of extras! Thanks.
Before, when using JavaScript, search engines would follow the links and vote; have you dealt with that?
I'm not crazy about the ability to undo votes because my goal is that eventually something like Vote Up/Down would replace this module and leaving that feature to them gives people an incentive to move.
I assume #4 is a result of #7?
Did you run Coder to make sure it adheres to Drupal coding standards?
#34
Yes, i've run it through coder, there few notices about using @see in comments, but i can't understand what's wrong with them :)
In the Up/Down users can just put "down" in my case users just can undo their vote. May be this feature was specific to my site... any ways there is an option to disable it.
In #4 i was talking about multiple widgets for same entity. in my case comments has to widgets, regular +1 and abuse. By default module adds plus1_comment_jquery_widget($entity_id, 'plus1_comment_vote') widget in the hook_comment_view(). And then its easy to create your own hook_comment_view() and add yet another widget plus1_comment_jquery_widget($entity_id, 'plus1_comment_abuse_vote');
And in #7 i was talking about hook which is called, when actual voting takes place.
regarding votes by search engine, in the theming function we have $can_vote variable, which is set by checking permission. If you'll provide anonymous user with permission to vote, probably search engine will vote too... probably workaround for this issue is to put rel="nofollow" for voting link.
#35
Ha! I have never figured out that @see problem either.
Undoing votes has definitely been asked for, but as I explained, I am reluctant.
I don't remember if the person who submitted the patch to dump JS tried nofollow or not. At any rate it has been removed from the 6.x branch.
There have been lots of people posting issues about anonymous voting, so I would assume it is quite common.
#36
Looks like there are at least two different D7 ports of this module already, the first one on github, the second one in a monster tarball that additionally changes more than what should be changed in a port (but of course, separate tasks/feature issues are fine). :(
Any chance to get anything into the official repo on d.o, so people have a clear target for coding and development?
#37
@sun, as soon as I can learn Git well enough to get back to doing some maintenance, I'd be happy to.
#38
@sun, yep i agree that its a bit more then just a port :) But Drupal 7 stepped forward too. I think this module should use entities instead of nodes only. This is first step, where nodes/comments/taxonomy_terms are used. But any ways in future releases module need to be rewritten in order to support any entity type via hooks.
#39
Just a heads up -- Voting widgets should use the 'vote' tag unless they're explicitly told not to by administrators. The intent of the tag system is to allow votes to exist in a shared pool by 'Vote Type' unless they really, really need to be put into a separate pool.
Defaulting to 'vote' and allowing the admin to specify a different tag for a given widget is cool, but using a different one out of the box means that a user can never switch from, say, Plus1 to VoteUpDown or vice-versa. The primary idea behind VotingAPI is that vote data should be 'widget-agnostic' as long as it's of the correct data type.
Other than that, it's great to see some action on Plus1...
#40
@zhgenti: I'm not saying that the additional changes are unwanted. I'm merely saying that it's a common practice to port contributed projects "as simple as possible" for new versions of Drupal core - primarily to be able to get them out of the door as soon as possible. Further tweaks and improvements may still land in the same major version, some others may not and might require a new major version branch, but that's just natural evolution. The ultimate goal, however, should be to not have existing users locked into a previous version of Drupal core (preventing entire site upgrades), or preventing adoption of a new Drupal core due to missing contribs. Thus, KISS, and always try to move forward in small steps.
So to move forward, we should do exactly that - small steps.
However, @eaton's initial port is >1 year old, and it's not clear whether the current code for D6 contains any fundamental changes that need to be incorporated. Additionally, I don't see an easy way to get a diff of @ReneB's changes on http://gitorious.org/plus1-d7/plus1-d7/trees/master
If we manage to get a patch of this basic porting work up and running, and committed, then there'd be a solid development target to develop against again. It sounds like @NancyDru could use some help with git along the way, so we might want to help her a bit out on IRC :)
Note that I only stumbled over this issue by accident, while searching for voting modules available for D7 -- since I don't have a use-case for the module myself right now, I don't think I'll have time to work on it, sorry :-/
#41
If we get a patch that everyone agrees on then I can fix the git stuff - no problem. I just haven't got the time to do any reviews on this project lately.
#42
subscribing
#43
So maybe better to start 2 branches: 7.x-1.x as direct backport and 7.x-2.x as entity-related ?
@zhgenti you can use sandbox to put your code, and thanx for your work
#44
subscribe
#45
+1
#46
+1 ..
#47
Subscribe
#48
@nancy, I will be more than happy to help you with git issues to sort this out. Just say the word. If you want to give me temporary git access, I would also take the various threads here and pull them in as separate non-release branches so it's easy to diff between them, etc.
#49
I put the two lines of work into a sandbox. The sandbox project page explains what branches correspond to which issue numbers.
@NancyDru, To get these as-is into the main repository (here) so I can delete that sandbox:
cd plus1git remote add sandbox git://git.drupal.org/sandbox/rfay/1154346.git
git fetch sandbox
git checkout --track sandbox/ReneB_506936_10
git checkout --track sandbox/zhgenti_506936_32
git push --all origin
or I'll do it for you or help/coach you live if that helps.
#50
I note that neither of these versions currently lets anon users vote, despite the existence of a permission.
#51
Thanks, Randy. You're on. I'll be at a customer site on Thursday. but I'm flexible on Friday. And just to punish you for offering, you are now a maintainer too.
#52
I'll be flexible on Friday as well, so let me know what works for you. I'm usually in IRC, and feel free to email me, randy at randyfay dot com as well. If you want to do the push, let me know. If you want me to just push it up, I'll do that (and there will be plenty left to do :-)
#53
Do I need to remove Tortoise CVS first? I suppose there's no need for it any more. I'll try again to install Tortoise Git before that.
#54
You do not need to remove tortoise CVS. If you'd like to use Tortoise Git, install it (with msysgit).
#55
Ah it lives.... again.. :) The reason i stopped working on porting was partly awaiting the big D.O to git migration and the lack of a 'official' D7 trunk.
However if still needed, i'am more then willing to merge/help/work on this project if needed :)
#56
OK, we didn't get to meeting on Friday, so I just went ahead and committed those two branches to this project. They're there in Git now.
Attached is the diff from zhgenti's version to ReneB's version. zhgenti obviously added a lot of features and seems to have worked with this extensively. It also appears that his work was directly based on ReneB's patch.
I guess as a naive user (I haven't worked on this code) I'd be prone to just go with the zhgenti version and call it 7.x-1.x. What do you think?
You can review the zhgenti code even without git at http://drupalcode.org/project/plus1.git/tree/refs/heads/zhgenti_506936_32. If you'd like to take it for a test run, here's the zhgenti snapshot (and here is the ReneB snapshot). But I'd love to get a D7 branch opened for this, with a dev release.
#57
+++ b/jquery.plus1.js@@ -1,18 +1,19 @@
+ plus1_widget.find('.plus1-link').attr('href', function(){ return $(this).attr('href') + '&json=true'; }).click(function(){
+ $.getJSON($(this).attr('href'), function(json){
Needs to be upgraded to D7 #ajax.
+++ b/plus1.admin.inc@@ -0,0 +1,222 @@
+ $vocabularies = array();
+ foreach (taxonomy_get_vocabularies() as $voc) {
+ $vocabularies[$voc->vid] = $voc->name;
+ }
+
+ $form['plus1_taxonomy_term_widget_settings']['plus1_taxonomy_vocabularies'] = array(
+ '#type' => 'checkboxes',
+ '#title' => t('Add voting widget to following vocabularies'),
Looks like all of these settings should be either per-bundle settings (living on their respective dedicated UIs), or per-field(-instance) settings (integrating with field widget settings).
+++ b/plus1.info@@ -1,12 +1,6 @@
-package = Voting
...
+package = Other
"Other" should never be specified as package. Not sure why "Voting" was changed - I think that most voting modules are using that package.
+++ b/plus1.module@@ -1,183 +1,234 @@
+include_once(drupal_get_path('module', 'plus1') . '/theme/theme.inc');
Unconditionally including an include file does not make sense at all. If it's unconditionally included, then you can as well put the code right into the .module file, and thus, micro-minimally increase performance due to skipping a filesystem lookup.
+++ b/plus1.module@@ -1,183 +1,234 @@
function plus1_permission() {
return array(
- 'vote on content' => array(
- 'title' => 'Vote on content',
- 'description' => 'Cast votes on site content using the Plus1 voting widget.',
+ 'plus1 vote on node' => array(
+ 'title' => t('Vote on nodes'),
+ 'description' => t('Cast votes on site content using the Plus1 voting widget.'),
+ ),
+ 'plus1 vote on comment' => array(
+ 'title' => t('Vote on comments'),
+ 'description' => t('Cast votes on site comments using the Plus1 voting widget.'),
),
- 'administer the voting widget' => array(
- 'title' => 'Administer the voting widget',
- 'description' => 'Make configuration changes to the Plus1 voting widget.',
+ 'plus1 vote on taxonomy_term' => array(
+ 'title' => t('Vote on taxonomy terms'),
+ 'description' => t('Cast votes on taxonomy terms using the Plus1 voting widget.'),
+ ),
+ 'administer the plus1 voting widget' => array(
+ 'title' => t('Administer the voting widget'),
+ 'description' => t('Make configuration changes to the Plus1 voting widget.'),
),
);
...
+ 'access arguments' => array('plus1 vote on node', 'plus1 vote on comment', 'plus1 vote on taxonomy_term'),
...
+ 'access arguments' => array('plus1 vote on node', 'plus1 vote on comment', 'plus1 vote on taxonomy_term'),
1) Missing module update for changed permission names.
2) D7 has built-in support for arbitrary entities, so defining only a few static permissions for some core entities will extremely limit this module. Brings me back to the settings issue I raised earlier: looks like we're still missing an essential entity-abstraction.
+++ b/plus1.module@@ -1,183 +1,234 @@
+ 'access callback' => 'plus1_user_access',
'access arguments' => array('administer the voting widget'),
The custom access callback is superfluous here and can be removed.
+++ b/plus1.module@@ -1,183 +1,234 @@
+function plus1_user_access() {
+ global $user;
...
+ if ($user->uid == 1) {
+ return TRUE;
+ }
Check for uid 1 can be removed, since user_access('foo') always resolves to TRUE for uid 1.
+++ b/plus1.module@@ -1,183 +1,234 @@
+function plus1_undo_vote($entity_type, $entity_id, $tag = 'plus1_node_vote') {
The addition of a 'undo' feature should be reverted and re-done in a separate issue. This issue should not introduce any new features that are not directly related to porting against core API changes.
Powered by Dreditor.
#58
I asked sun in IRC
and he said:
#59
@sun yes, your right regarding this:
as i wrote in http://drupal.org/node/506936#comment-4286508 its was just i first step, to support entities, which were needed in my project.
#60
I think it would be grate to write small module for converting votes with tags. It will allow to keep vote as general instance and change voting widgets from one to another. When we use no tags, its impossible to use few widgets with one node, but there are cases when its needed. What do you think?
its reply to http://drupal.org/node/506936#comment-4286644
#61
OK, I branched zhgenti's as 7.x-1.x, and created a dev release. We'll be working against that now. Looking forward to a patch resolving the items in #57. You can now use 7.x-1.x to work against.
#62
@zhgenti: Vote as a general instance sounds somewhat related to some larger development that's currently happening in http://drupal.org/sandbox/sumsi/1074206
#63
#64
@sun: i was talking about just changing vote tags from one to another to deal with:
Do you think http://drupal.org/sandbox/sumsi/1074206 will replace VotingAPI?
#65
+1
#66
Progress == good :) I will remove my version from gitorious, to prevent it from surfacing somewhere again in favor of zhgenti's version :)
#67
sub
#68
sub
#69
sub
#70
subscribing
#71
Hi,
Unfortunately I had no time to address issues mentioned in the comment #57.
But here is small improvement. Now Render API is used, js and css are attached via "#attached" property as long as adding these files in the preprocess or theming function doesn't work when node/comment are cached with Render API.
If you need to attach your own js/css you should use alter hook like this:
<?phpfunction hook_plus1_widget_alter(&$widget, $context = NULL) {
if ($widget['#can_vote']) {
$widget['#attached']['js'] = array(path_to_theme() . '/js/jquery.plus1_custom.js');
}
}
?>
#72
I recommend granting @zhgenti commit privileges if willing...
#73
Done.
Welcome to the team.
#74
hope to see the D7 release soon.
#75
There is a dev release already; you can test it.
#76
1) Patch didn't apply to 7.x-1.x (one failed hunk in plus1.module)
2) Called plus1_get_cleared_destination() which doesn't exist. Replaced with drupal_get_destination(). I'm guessing you have that function locally but didn't add it to the patch...
3) #attached syntax was wrong (missing array), leading to more errors.
So here's a reroll taking that into account. Seems to work now.
#77
Did you apply that patch against latest dev version?
plus1_get_cleared_destination should be there, but any ways, guys just gave me commit access and i will handle this tonight.
Thanks
#78
Yes, tried both the -dev version and a git checkout of 7.x-1.x.
In any case, the module now fullfils my use case. Thank you zhgenti for your efforts, they are appreciated.
#79
#71 was rolled against an entire site, instead of against a plus1 repository. However, even when applied with patch -p5 it didn't apply. @zhgenti please do make sure you're working against this repository!
@zhgenti I'm not the maintainer here, but I'll ask what I generally ask of all maintainers: Work openly in the issue queue, try to get reviews of your patches (at least post them), make sure that nearly every commit has a matching issue comment, post the hash or link of the commit in the issue when you commit, so your work can be correlated.
#80
I also ask that all patches be after running through the Coder module so that the code meets coding standards. I also prefer that any potential requisites be discussed before being coded.
Randy, you are a committer.
#81
hi,
just committed changes from #71 with corrections mentioned by @bojanz in #76. Commit hash is cbb1b5
code was checked with coder, but i can't get rid of @see warning again...
Thanks,
Dmitry
#82
I think the @see warnings are incorrect; just ignore those.