Closed (fixed)
Project:
Simplenews Roles
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
29 Sep 2011 at 08:42 UTC
Updated:
1 Apr 2012 at 14:50 UTC
Jump to comment: Most recent file
Comments
Comment #1
joachim commentedCool!
Here's a quick review on reading the code; not run it through its paces yet.
> module_load_include('module', 'simplenews', 'simplenews');
Won't simplenews load that? If not, better and more efficient to load it only when we know we need it rather than every page load.
> function simplenews_roles_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
hook_nodeapi() has gone in D7!
> if ($node->type == 'simplenews') {
Does SN on D7 limit us to one type of node? In the 6--2 branch of SNR this picks out node type from config.
> * Implements hook_user().
Another hook that has gone for D7!
Looks like this needs more work. Will need the tests file upgrading too.
I've made a few basic code clean-ups for formatting and standards -- here's a patch of your work + my tweaks rolled against the 6.x-2.x branch.
Comment #2
aacraig commentedI wasn't sure how much work had already been done on this, so the version I submitted is admittedly quick and dirty.
Basically, I enabled the D6 version in a D7 install and fixed all the errors :)
I'm happy to do a more thorough porting if you are confirming that no one is actually working on this at the moment, building on the patch you included in your post.
Comment #3
miro_dietikerGreat to see you guys having interest in porting this module. A clear D7 statement aund improved functionality would be great.
Anyone of you to take over development lead of the "simplenews roles" for the following time?
A D7 version might take some time and we'll need some more support. Will you find time?
Comment #4
aacraig commentedBelieve it or, I've had this on my task list for a couple of months to come back and look at fixing up the D7 version, but obviously haven't gotten around to it yet.
I'll be happy to head up the D7 version, though I won't be able to put any time into it until the end of the year. I think a good D7 port for Jan 2012 is a realistic goal, though.
Comment #5
joachim commentedI'd be very happy to add co-maintainers to this project. Though please make use of Coder module until Drupal coding standards are ingrained :D
Comment #6
extrarumeno commentedSubscribe ..
D7
Comment #7
joachim commentedJust had another quick look at this.
simplenews_roles_update_subscriptions() should probably not be called from anywhere other than cron runs, as it's potentially a very database-heavy operation if there are a lot of users.
Also, all of my comments that explain the queries in that function seem to be getting removed in the patch!
Comment #8
rhcp commentedHello!
I have a small problem.
When I run the task scheduler (cron), I get an error.
Undefined index: context in simplenews_roles_mail_alter ()
119 online. I have not yet patched
Comment #9
joachim commentedPlease open new issues for different bug reports rather than comment on existing ones.
Comment #10
miro_dietikerWe're currently working on a clean D7 port (recoded) that scales well with 100k subscribers / users.
It's already working and internal reviews/cleanup take place.
We have also great test coverage already.
I'll pass you the result ASAP.
Comment #11
miro_dietikerPushed a D7 version after some coding cycles and review processes.
This time including test coverage. :-)
Comment #12
miro_dietiker