Posted by davidneedham on May 17, 2011 at 12:27am
13 followers
| Project: | Notify |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Issue Summary
Do you have any plans to update this module to Drupal 7?
Comments
#1
Yes, after two recent projects with Notifications module, I have a renewed appreciation for the need for a simple module in the niche that Notify occupies.
I think, I'd like to make it Views-centric; i.e., you create a view and whatever is in that view is what gets mailed.
However, I the issue is lack of time. I'm booked solid for the next 3 months, at least. I'm open to accepting co-maintainers who want to pick up the slack.
#2
Subscribing
#3
#4
Would you be interested in a straight port? I need something simple for a site I'm working on and I think Notify fits. While basing it on Views sounds lovely and would be a great feature, I don't have a lot of time. If you'd be interested in a straight port, I might have a go at it. No promises at this point... I'm still investigating options. But I wanted to at least put the thought out there.
Michelle
#5
Hi Michelle,
I have created a D7 branch at http://drupal.org/node/1227682 feel free to test and patch.
#6
Wow, that was fast! I'm having no luck with any other angle I've tried so this may be my best bet.
Michelle
#7
Attached is a patch to the 7.x DEV branch that creates a working version. It fixes several database errors and other issues within that first attempt. It was nice not having to start from scratch, so many thanks to Ishmael for getting this started. I also attached a tar with the full module for anyone that wants the whole thing.
#8
#7:
Fatal error: Cannot use object of type stdClass as array in /home/cmseasyn/public_html/drupal-7-dev/sites/all/modules/notify/notify.module on line 689
#9
Found a few issues. Here's a new patch building on #7, with a few changes.
Includes:
#10
Another patch based on the patch of #9.
Changes:
When I open config/people/notify/users, the usernames and email where not visible. I might have something to do with this: http://drupal.org/node/1261040. I changed the type to a disable textfield to show something.
I have been using notify with these patches for a couple of weeks on a D7 setup with problems. It might be time for an alpha release of the D7 port?
#11
An additional patch on top of #10. It gives a better output of fields in a mail.
#12
Here is the D7 port of module Notify named Notify2.
http://drupal.org/sandbox/WebEvt/1460518
#13
Is this different than the dev version dated jun 2011 at the project page? I suspect it is. How do I download it? I clicked on the above git link but it took me to a url that didn't get me far. I'd like to try out this version if possible?
Thanks
#14
?
#15
IWasBornToWin, download the git repository, check out the 7.x-1.x-dev branch and apply the patches found in this thread.
#16
@wpoely,
Could you make one cumulative patch to make reviews easier? Then if scottrigby or john.oltman or someone else can give a positive review, I'll commit it.
#17
@matt2000,
In attachment is a patch with all changes from this thread and it applies cleanly to the 7.x-1.x branch. One patch is git aware, the other one is just a diff. I've split it up in serveral patch to make it looker cleaner.
We've used notify with this patch on D7 for the last 6 months without problems.
#18
@wpoely86, @matt2000,
I run Drupal 7.15. I've installed the current 7.x-1.x-dev (2011-Jul-24) and applied notify-d7port-1159632-17.patch.
Running patch produced the error message below:
patching file README.txtpatching file notify.info
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file notify.info.rej
patching file notify.install
patching file notify.module
I edited notify.info "by hand" by adding the three lines that patch were unable to add, and that seemed to work out OK. At least, installing the patched version on several of my D7 sites went without a glitch.
So as far as I am able to tell, this version (with the patch) works well with Drupal 7. So there is a positive review for this patch from me (for whatever that is worth).
Notify fills a niche that (AFAIK) no other notification module fills. It is perfect for admins (and others) that only want a lightweight module that sends out email notifications about new/changed content on a fairly static site.
I vote for getting this patch into the the development release, since it is a improvement over the current development version and I haven't spotted any real problems with it. (There are minor nits, such as the configuration menu not appearing under admin/configiration, and missing defaults for some of the radio buttons in the UI, but I'll wait submitting issues/patches for those until after a new development release is available, so not to make the patch queue more difficult to follow.)
As an enthusiastic user of the Notify module (and I know of no real alternative), I also hope the maintenance status of the module will be changed from "Unknown" to "Actively maintained".
#19
@gisle,
the patch is for the git 7.x-1.x branch and should apply cleanly on it.
Can you elaborate a bit more on the nits? The configuration menu should appear under Configuration/People/Notification Settings? Which defaults are you missing?
We can better fix all the nits now and be done with it...
#20
@wpoely86,
I am sorry. I simply misunderstood what you meant by the "7.x-1.x branch". I tried to apply the patch to the the
notify-7.x-1.x-dev.tar.gztarball downloaded from http://drupal.org/node/1227682. That didn't work. If I instead use git to clone the 7.x-1.x branch to my local disk, the patch applies cleanly.You're also right about the configuration menu appearing (as it should) under Configuration/People/Notification Settings. Sorry about overlooking it.
Main nit:
I am able to run the patched version of the D7 port without problems as long as I do not enable Notify new comments for any user. If I enable this, and there are comment notifications in the queue, Notify crashes cron with the following fatal error:
Fatal error: Cannot use object of type stdClass as array in /www/www.elvegaarden.net/htdocs/sites/all/modules/notify/notify.module on line 695As a quick "fix", disabling Notify new comments for all users makes Notify operational for Notify new content.
My only remaining nit is that the radio buttons in the UI has no default values set when they initially appear - see the screen dump below:
I think they should be set to indicate the following default values: Disabled, Disabled, Title only, Disabled.
#21
Digging some more into notify, I've noticed that Notify sets up new users with sensible defaults if a user register after you've enabled Notify. So if you enable Notify before any users register, you will not see the form in the state shown in the screenshot #20 (i.e. all the radio buttons are unset)
However, Notify does not touch users that already exist when you enable it. So all existing users will be presented with the form in the state shown in the screenshot #20 (i.e. all the radio buttons are unset). If the user then just presses "Save settings" in this state, the following PDOException is triggered:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'status' cannot be null: INSERT INTO {notify} (uid, status, node, teasers, comment) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4); Array ( [:db_insert_placeholder_0] => 4 [:db_insert_placeholder_1] => [:db_insert_placeholder_2] => [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => ) in notify_user_settings_form_submit() (line 392 of /hom/inf5270/www_docs/diw/gruppe01/sites/all/modules/notify/notify.module).Quick "fix": Tell the users to make sure that they've actually choosen something in all radio button groups before clicking "Save settings".
I've made a fix that just checks the state of the notify object before building the form, and if
$notify->statusis not set, it quietly adds sensible defaults. I'll upload a patch here shortly.A better solution would be to make Notify take care of existing users during installation, but for the moment, I think the solution described above has to do.
Changed the Status to "Needs Work".
#22
OK. I've prepared a patch for the D7 port of notify that includes the following changes:
$notifyobject is never NULL, even when the form is viewed by pre-existing users (see #21 for description of problem).The patch is attached, and should be applied to the current 7.x-1.x branch in git.
This fixes all the "nits" I've had with the D7 port of Notify. Please review.
(Changed status to "needs review".)
#23
I /think/ this is all good stuff, but it's hard to review the patch quickly because it mixes code-style clean-up with functionality changes.
Also, the fix for #935458: $nodes array modified inappropriately by comment notification logic in _notify_send() should be in that issue queue as it's own patch.
Would it be easy for you to split it into a few seperate patches?
#24
OK, I've split the patch in two.
The first (
notify-code_standard-patch_1.git.patch) only changes the formatting to pass automatic code review at http://ventral.org/pareview. I.e. cleaning up the code to match the D7 coding standard. There is no change in logic. The patch should be applied to the current 7.x-1.x branch in git.The second (
notify-bugfix-patch_2.git.patch) does the following:This patch should be applied immediately after applying the first patch (above).
@matt2000, on Sun, 11 Nov 2012 13:48:21 +0000 (UTC), i sent you a PM using the contact form on this site. Did you receive it?
#25
#26
gisle,
I've looked through your patches and they look fine. I'm currently tracking all changes to the 7.x branch on github: https://github.com/wpoely86/notify
@matt2000, I think we are ready for a beta release?
#27
Looked at your 7.x branch on github (https://github.com/wpoely86) and there is one small thing.
I believe that the recommended practice is to not use the
t()function insidehook_menu()(see http://drupal.org/node/323101 for the reason).Otherwise, the HEAD looks fine.
#28
OK, I didn't know that. I learned something new today :)
I've deleted the commit from github.
#29
Thanks!
After the change, the 7.x branch on github https://github.com/wpoely86/notify looks fine to me.
#30
The 7.x-1.x-dev release of Jan. 4, 2013 contains the following changes:
This addresses all outstanding issues for the 7.x-1.x branch.
I hope users of this module will download and test this release thoroughly. Please post reviews in this thread.
#31
The 7.x-1.x-dev snapshot pushed Jan. 13, 2013 contains the following changes:
#32
I am using this module for more than a month on production, and everything works as expected. I also did not come across the fixed bugs reported at #30 and #31.
Everything looks good to publish a stable release.
#33
I am also using this in production, and has not experienced any problems since the release of the dev version of 2013-Jan-13. Since Jan. 13, there have been no bug reports in the issue queue.