Closed (fixed)
Project:
Notify
Version:
4.7.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
6 Jun 2006 at 02:48 UTC
Updated:
9 Mar 2007 at 19:47 UTC
Jump to comment: Most recent file
Comments
Comment #1
RobRoy commentedI recently took this project over and have just returned from vacation. I'll take a look at the scheduler issue.
Comment #2
Eagle-i commentedIs there any progress? My users who used it, do not receive any notifications anymore and are asking me about it.
Comment #3
RobRoy commentedSo for this bug you have content that is scheduled to be published tomorrow, for example, but the content was created before Notify was installed/run so it is not catching it. I'd like to clear this up and I apologize for the delay. If anyone has some time, please feel free to submit a patch. If not, I will try to get to this soon.
Comment #4
Eagle-i commentedUpdate:
i discovered that my notify.module files were at zero (0) bytes on the server. No good upload. THat's fixed and should be working now. Sorry about that.
Comment #5
RobRoy commentedHehe, cool. Can you confirm that the bug with scheduler still exists? I have a feeling it does.
Comment #6
Eagle-i commentedI am not sure what you mean with the scheduler, but i use notify module with poormanscron module and they seem to be working fine.
Comment #7
kthagen commentedI have had this situation occur under the following conditions:
1. Content is added with a scheduled publish date in the future.
2. The publish date is edited so that it is (significantly) earlier than the current time.
3. The next time cron triggers, the content is published, but there's no notification.
Looking at the code of notify.inc and scheduler.module, I think the problem is with scheduler, not notify.
When scheduler publishes a node, it sets the node's created and changed fields to publish_on. That means that if the difference between now and publish_on is greater than notify's period, notify will ignore the content, assuming that notifications have already been sent.
The fix should be to change the relevant update query in scheduler.module. I'll post a patch (not here--wrong project) once I've tested it.
Comment #8
kthagen commentedOK, more testing indicates that there is a fix necessary for notify. The modules in the cron chain are sorted first in terms of module weight, and then by name. Since "notify" comes before "schedule" and both have the default weight of 0, the notification routine will always run before schedule publishes the new nodes. Then, since the publication date is set to something before the time of the cron run, it will be outside of our window on the next invocation of notify.
The following patch to notify.install modifies the system table so that notify has a weight of 9. This cause notify to run after almost all the other modules have done their work. Schedule uses the default weight of 0, so a lower value would work too, but this will put it after other modules, like project, too. Run update.php after patching.
You still won't get a notification in the case I outlined in my last message, or in some cases if notify's frequency settings are less than the cron interval for schedule, but it should fix the ordinary case. (I've tested it, and this works for me.)
Comment #9
RobRoy commentedNeed a re-roll against HEAD.
- Should initialize $ret = array();
- Should put it in a $GLOBALS['db_type'] switch, even if we fall through all three db types mysql/mysqli/pgsql.
- Need Doxygen above update to explain what the reasoning is for the update.
- Should dynamically add +1 to whatever the current weight for publisher is since a user may have manually changed that to 10. If it's 0 then 0+1=1 is fine.
Thanks for all your hard work kthagen!
Comment #10
kthagen commentedTwo points:
First, I'm not quite sure I follow what you mean by the second point. The update is standard SQL, so there's no need for a switch ($GLOBALS['db_type']) here. The general practice from the modules I've looked at, including the core system updates in database/updates.inc, seems to be to use that switch only if there's some mysql vs. postgresql difference.
Of course the same weight setting does need to be done in the _install, although again, I don't see why it should be in the switch.
Second, I think looking at scheduler's weight and picking the value from that has potential problems. A minor one is that if a user is going to reset the module weight manually after installing notify, the problem will recur. More importantly, if we give ourselves a weight of 1 (which is what will happen in the vast majority of installations), that's fine for scheduler, but what about some other module that publishes on the cron hook? It's easy to envision other modules, not yet in existence, that could assign themselves a weight >0.
I don't think the exact weight for notify matters much, but it should be fairly high. (I note that devel uses a weight of 88--perhaps less than that, but not by too much.)
I can have a new patch ready very quickly once I know your thinking on this, RobRoy.
Comment #11
RobRoy commentedThe first point really is just my preference, mostly for the sake of being explicit and educational purposes for others looking at our install code. But, it does look a bit silly and core updates don't even do this so I'm alright with it either way.
Yes, we should weight the module in hook_install() as well. Regarding the weighting, since we want to go AFTER scheduler, we really should be ensuring that. True, it's an edge case since I'm sure zero people have adjusted their module weights for scheduler. If there are other modules we want to go after, we should get those weights too and do a max(...) + 10 (this is a good point you brought up, to be on the safe side for any overlooked cron publishing modules).
So basically, neither of these things are super important. You can ignore the switch anad just hardcode the module weight to 10 or something and that'll be fine. But I slightly prefer the other way. My points #1 and #3 are the only definites.
Thanks.
Comment #12
RobRoy commentedOn second thought, you're right on the switch stuff. It's just cruft to leave that in there. So leave the weighting in _install and _update_1 out of the switch like you said.
Comment #13
kthagen commentedHere you go. The comments are a little chatty and might bear simplification.
Comment #14
RobRoy commentedI committed this to 4.7 and up a while back, forgot to mark fixed. Thanks kthagen!
Comment #15
(not verified) commented