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.
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.
Comment | File | Size | Author |
---|---|---|---|
#76 | utilizing-of-render-api-reroll-506936-71.patch | 11.85 KB | bojanz |
#71 | utilizing-of-render-api-506936-71.patch | 11.98 KB | zhgenti |
#56 | plus1.zhgenti_d7_version.patch.interdiff.txt | 45.44 KB | rfay |
#32 | plus1.zip | 16.81 KB | zhgenti |
#13 | plus1-nodetype.patch | 571 bytes | j0rd |
Comments
Comment #1
voxpelli CreditAttribution: voxpelli commentedExcellent! Subscribing to this one to make sure that I look further at it when I get time.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedAny chance we can get a commit and dev snapshot for plus1 d7? would help with my testing of module dependencies
Comment #3
voxpelli CreditAttribution: voxpelli commented@Moshe: Wouldn't it be good if someone took time to test the patch prior to committing it?
Comment #4
eaton CreditAttribution: eaton commentedThe 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.
Comment #5
eaton CreditAttribution: eaton commentedAnd here's the latest version -- the JS works, and everything seems to be humming along smoothly.
Comment #6
willmoy CreditAttribution: willmoy commentedI second that.
Comment #7
willmoy CreditAttribution: willmoy commentedTag
Comment #8
andypostsubscribe
Comment #9
NancyDruJeff, while I have no problems with patching, there are many who do. Perhaps for them you could attach a complete tarball or zip file?
Comment #10
Rene BakxMy 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
Comment #11
eaton CreditAttribution: eaton commentedAwesome!
Comment #12
j0rd CreditAttribution: j0rd commented@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
I've attached this diff and might commit to glitorus after more testing (if I have access to do that)
Comment #13
j0rd CreditAttribution: j0rd commentedI 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.
Comment #14
alpilein CreditAttribution: alpilein commented@ReneB Post #10:
I just found another minor issue. Following code can be found twice in "plus1_settings()" and can be removed once:
Comment #15
j0rd CreditAttribution: j0rd commentedI 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.
Comment #16
NancyDruTrigger/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.
Comment #17
NancyDru@eaton: Jeff thanks for this work. Would it be possible to re-roll with all the latest fixes?
Comment #18
NancyDruComment #19
OnkelTem CreditAttribution: OnkelTem commentedWhen we get dev version for D7? There is even no CVS branch for it in the CVS atm.
Comment #20
voxpelli CreditAttribution: voxpelli commented@OnkelTem: See #17 - there needs to be code to commit prior to a commit being possible :)
Comment #21
Rene BakxYeah 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 :)
Comment #22
Rene Bakx@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.
Comment #23
kinshuksunil CreditAttribution: kinshuksunil commentedsubscribing
Comment #24
andypost@NancyDru can you commit current work into 7-dev, it make much easy to polish code.
Comment #25
rodych CreditAttribution: rodych commentedSubscribing
Comment #26
achton+1 :)
Comment #27
NancyDruI am still working on getting set back up after I had my hard disk rebuilt, so I don't have a D7 operating yet.
Comment #28
craigritchie CreditAttribution: craigritchie commentedSubscribing
Comment #29
wrd CreditAttribution: wrd commentedHaving 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.
Comment #30
triskele CreditAttribution: triskele commentedI'm looking forward to a dev release. That is music to my ears.
Comment #31
NancyDruNo -dev release while this is marked "needs work."
Comment #32
zhgenti CreditAttribution: zhgenti commentedHello 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
Comment #33
NancyDruWhew, 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?
Comment #34
zhgenti CreditAttribution: zhgenti commentedYes, 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.
Comment #35
NancyDruHa! 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.
Comment #36
sunLooks 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?
Comment #37
NancyDru@sun, as soon as I can learn Git well enough to get back to doing some maintenance, I'd be happy to.
Comment #38
zhgenti CreditAttribution: zhgenti commented@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.
Comment #39
eaton CreditAttribution: eaton commentedJust 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...
Comment #40
sun@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 :-/
Comment #41
voxpelli CreditAttribution: voxpelli commentedIf 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.
Comment #42
Jiri Volf CreditAttribution: Jiri Volf commentedsubscribing
Comment #43
andypostSo 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
Comment #44
rwohlebsubscribe
Comment #45
hendrakieran CreditAttribution: hendrakieran commented+1
Comment #46
yugongtian CreditAttribution: yugongtian commented+1 ..
Comment #47
Tesmon CreditAttribution: Tesmon commentedSubscribe
Comment #48
rfay@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.
Comment #49
rfayI 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:
or I'll do it for you or help/coach you live if that helps.
Comment #50
rfayI note that neither of these versions currently lets anon users vote, despite the existence of a permission.
Comment #51
NancyDruThanks, 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.
Comment #52
rfayI'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 :-)
Comment #53
NancyDruDo 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.
Comment #54
rfayYou do not need to remove tortoise CVS. If you'd like to use Tortoise Git, install it (with msysgit).
Comment #55
Rene BakxAh 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 :)
Comment #56
rfayOK, 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.
Comment #57
sunNeeds to be upgraded to D7 #ajax.
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).
"Other" should never be specified as package. Not sure why "Voting" was changed - I think that most voting modules are using that package.
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.
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.
The custom access callback is superfluous here and can be removed.
Check for uid 1 can be removed, since user_access('foo') always resolves to TRUE for uid 1.
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.
Comment #58
rfayI asked sun in IRC
and he said:
So sounds like the vote is to go with zhgenti, removing the one feature called out by sun.
Comment #59
zhgenti CreditAttribution: zhgenti commented@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.
Comment #60
zhgenti CreditAttribution: zhgenti commentedI 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
Comment #61
rfayOK, 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.
Comment #62
sun@zhgenti: Vote as a general instance sounds somewhat related to some larger development that's currently happening in http://drupal.org/sandbox/sumsi/1074206
Comment #63
sunComment #64
zhgenti CreditAttribution: zhgenti commented@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?
Comment #65
rogical CreditAttribution: rogical commented+1
Comment #66
Rene BakxProgress == good :) I will remove my version from gitorious, to prevent it from surfacing somewhere again in favor of zhgenti's version :)
Comment #67
theullrich CreditAttribution: theullrich commentedsub
Comment #68
witchcraft CreditAttribution: witchcraft commentedsub
Comment #69
mrdavey CreditAttribution: mrdavey commentedsub
Comment #70
sw3b CreditAttribution: sw3b commentedsubscribing
Comment #71
zhgenti CreditAttribution: zhgenti commentedHi,
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:
Comment #72
rfayI recommend granting @zhgenti commit privileges if willing...
Comment #73
NancyDruDone.
Welcome to the team.
Comment #74
rogical CreditAttribution: rogical commentedhope to see the D7 release soon.
Comment #75
NancyDruThere is a dev release already; you can test it.
Comment #76
bojanz CreditAttribution: bojanz commented1) 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.
Comment #77
zhgenti CreditAttribution: zhgenti commentedDid 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
Comment #78
bojanz CreditAttribution: bojanz commentedYes, 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.
Comment #79
rfay#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.
Comment #80
NancyDruI 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.
Comment #81
zhgenti CreditAttribution: zhgenti commentedhi,
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
Comment #82
NancyDruI think the @see warnings are incorrect; just ignore those.
Comment #83
rogical CreditAttribution: rogical commentedpatch in #76 seems obsolete.
error: patch failed: theme/theme.inc:49
error: theme/theme.inc: patch does not apply
I think we can close this and focus on new issues of 7.x branch?.