Problem/Motivation
When updates are available, no notifications are sent.
Fixed in Drupal 7.
Separate issue in Drupal 6
Remaining tasks
This issue has been silent for almost two years. So:
- Check if still an actual issue.
- If so, write new patch.
- If so, write tests.
User interface changes
None
API changes
Unknown
Original report by @tomcashman
I have a Drupal 7.7 installation that sends no notifications when updates are available to modules or Drupal core, despite the fact that:
- Cron is running correctly every hour.
- The site successfully sends emails for other purposes, so
drupal_mail
is fine. - Update notifications appear in the administration pages when updates are available.
- The settings in 'Available updates' include:
- Check for updates daily
- Check for updates of disabled modules and themes
- Correct working email addresses are given for notifications
- Email notification threshold is 'All newer versions'
The expected behaviour from the above configuration is that the listed email addresses receive notifications when new updates are available. The actual behaviour is that no notifications are sent. There are similar reports at http://drupal.org/node/1039812
Drupal version: 7.7
OS: FreeBSD 8.1-RELEASE-p2
Database: MySQL 5.1.54-log
Web server: Apache/2.2.17 (FreeBSD) mod_hcgi/0.8.0 mod_ssl/2.2.17 OpenSSL/1.0.0c DAV/2
PHP: 5.3.5
It seems as if this issue has been fixed and committed for Drupal 8. However, there's a similar issue for Drupal 6 that is solved with the same solution at https://drupal.org/node/359171
Comments
Comment #1
dddave CreditAttribution: dddave commentedSetting it to dev so that somebody cares... ;)
Comment #2
akamaus CreditAttribution: akamaus commentedI'm experiencing this issue too on all my D7 sites so I've made a quick research.
This is a hook_cron of update module:
The first check for updates always goes through the first branch, as update_last_check variable is non-existent.
But update_refresh() only enqueues the fetching task and _update_cron_notify only analyzes the cached results. So look like the fetching procedure isn't called at all. I tried adding a call to update_fetch_data in between and I immediately received an email notification.
On subsequent cron runs data seem to be fetched, at least _update_process_fetch_task is called. But what puzzles me is how the email notification is supposed to be sent. If cron runs more often than update_check_frequency, we never get to the first branch. But it's the only place in the module, where _update_cron_notify is called.
Hope someone, who better understands how all this is supposed to work, will give some hints.
Comment #3
tomcashman CreditAttribution: tomcashman commentedI believe
_update_cron_notify()
is supposed to send the notifications:I understand this as
$params
whether there are updates available for'core'
and'contrib'
.$params
indicates there are updates available, check the list of emails to notify.But on my installation, something must go wrong before the emails get sent. I don't know enough to guess what that might be.
Comment #4
akamaus CreditAttribution: akamaus commented@tomcashman, yes it is _update_cron_notify(). But who should call it and when? All the logic of update_cron looks highly suspicious to me.
Comment #5
akamaus CreditAttribution: akamaus commentedThis is hook_cron from D6 update
Here the logic is crystal clear. Checking and reporting always occur together. The info is fetched only when enough time elapsed from last check elapsed.
Can someone explain what was the logic behind the D7 rewrite?
Comment #6
tomcashman CreditAttribution: tomcashman commented@akamaus: Given your pointer that
update_fetch_data()
is what is needed to actually call the fetching procedure, it looks like the patch in this issue: #647964: Running cron does not check for available updates might fix this bug too? The patch does exactly what you suggest: addsupdate_fetch_data()
betweenupdate_refresh();
and_update_cron_notify();
.Comment #7
akamaus CreditAttribution: akamaus commented@tomcashman, thanks for the link. I suspect proposed patch solved only a part of problem though. Say, I run cron once a hour. It checks for updates and sets update_last_check to current time.
But then, given the interval is a day or a week, how this is supposed to evaluate to true?
And it's the sole code path there mail is sent.
Comment #8
AndrzejG CreditAttribution: AndrzejG commentedI don't know if it helps, but after update Drupal to 7.7 version from earlier versions the variable update_notify_emails corrupts in certain (not clear) situations.
Please see http://drupal.org/node/1284364#comment-5194882, and also compare some other issues on this variable.
Comment #9
akamaus CreditAttribution: akamaus commented@AndrzejG
Looks like I've experienced it too after upgrade to drupal-7.8.
Comment #10
tomcashman CreditAttribution: tomcashman commentedThis doesn't seem to be the problem in my case. Installing the Variable Check module gives a report with "No invalid variables found", and update_notify_emails looks fine to me in the variable database table.
I'm now using Drupal 7.9, and still get no update notifications at all.
Comment #11
webdevandy CreditAttribution: webdevandy commentedIs there any news on this? I noticed today that there were security updates for some modules on my site, even though I hadn't received any notifications.
Due to the fact that this bug stops people from receiving security notifications, should this issue be changed to critical?
Comment #12
StephenRobinson CreditAttribution: StephenRobinson commentedditto, with all versions of D7 up to 7.10.....????
Comment #13
StephenRobinson CreditAttribution: StephenRobinson commentedI commented out line 317 of update.fetch.inc and it started working, $params was empty, now it isnt, then it mailed the way it should
will invesigate further...
Comment #14
StephenRobinson CreditAttribution: StephenRobinson commentedsomething not right here, added some debug:
Comment #15
StephenRobinson CreditAttribution: StephenRobinson commentedI am alternating between the following 2 results:
Run one:
run two:
Comment #16
StephenRobinson CreditAttribution: StephenRobinson commentedso the problem is coming from
Comment #17
StephenRobinson CreditAttribution: StephenRobinson commentedI have traced this issue down to update.install, _update_requirement_check
commenting out lines 128 and 132 gets rid of the issue
Comment #18
StephenRobinson CreditAttribution: StephenRobinson commentedComment #19
StephenRobinson CreditAttribution: StephenRobinson commentedThis gets very confusing, using a simple update_cron to keep it running on each time:
The cron works every other time, in update_get_projects()
$projects = &drupal_static(__FUNCTION__, array());
is empty and gets rebuilt one time, the next it has data, but none of these entries contain a status???
Comment #20
StephenRobinson CreditAttribution: StephenRobinson commentedI give up...............works every other cron if I comment out both" if (empty($projects)) {" in update_get_projects() in update.compare.inc with my update_cron modified to run everytime for testing....
Comment #21
akamaus CreditAttribution: akamaus commentedAfter so much time still broken for me. Updates are checked now, but not emails are sent.
I looked a bit more into the code,
update_cron
logic looks broken to me.Let's look at it again:
If cron wasn't run for long time, the first branch is active, updates are checked and the mail sent. You can test it by setting
update_last_check
for a value in past. BUT if your cron job runs more often, thanupdate_check_frequency days
, then this will be the sole and last mail you get.Indeed, next cron runs else branch is activated,
update_get_available
function is called, which in turn callsupdate_create_fetch_task
which sets update_last_check to now. This loop will continue forever as long as cron is alive. No messages would be sent.I propose to untie email notifications sending and checking updates. For example, I'd like to check for updates every day and receive a mail as soon as security update is fetched, but after that I'd like get reminders once a week, not more often.
It can be implemented just in several lines. What do you think?
Comment #22
catchComment #23
akamaus CreditAttribution: akamaus commentedI've realized idea I described in #21, I've added additional variable to track last time the message was sent and decoupled fetching updates and message sending.
Works fine for me. Any comments?
Comment #24
catchCould we use the flood system for this instead of a variable?
Comment #25
catchWell update is already using variables for this, so probably doesn't make sense to do that here. The patch will need to be re-rolled against 8.x per http://drupal.org/node/767608 though, although it's likely to just be the path change in this case, so tagging as novice for the port.
Comment #26
akamaus CreditAttribution: akamaus commentedI fixed the paths so patch applies on D8 now. I couldn't run D8 on my environment and test it, but I studied diffs of update module between 7.x and 8.x and found no cardinal changes. So patch should work as is.
Comment #27
catchSmall update to the D8 patch just to fix comments/code style issues.
Also attaching a D6 backport - the D8 patch still applies with fuzz if you use P2 fwiw.
Comment #28
catchOne comment fix didn't actually make it into the patches...
Comment #29
akamaus CreditAttribution: akamaus commentedCan we hope it will be merged in a nearby future?
Comment #30
catchBetter D6 backport.
@akamus the only way this will get merged is if someone reviews the patch and marks it RTBC. Have you tried the patch?
Comment #31
jcisio CreditAttribution: jcisio commentedIt is not really related, but there is:
and in each _update_process_fetch_task() there is a variable_set() which causes performance problem.
Also, _update_cron_notify() is called twice (within some conditions) in update_cron(). I think the first one could be removed.
Comment #32
akamaus CreditAttribution: akamaus commented@catch, Yes, I tested it on D7, but since I'm the author of patch #23, I don't feel it's appropriate for me to mark it RTBC.
I maintain a couple of dozens of D7 sites and I'm bothered by the fact I receive no notifications about critical updates, looks like a security related issue to me on it's own... Surprisingly, it received almost no attention for about a year.
Comment #33
luk.stoops CreditAttribution: luk.stoops commentedHi, I did some testing on the patches #23 and #28 with strange results.
I am new in drupal and php so let me describe the steps I followed and the result:
Set up my repository:
$ git clone --recursive --branch 8.x http://git.drupal.org/project/drupal.git
import the site in my acquia dev desktop environment, set up and test the drupal8 site, especially the behavior of cron at /admin/config/system/cron
download and apply patch #28
$ git apply –v 1263040-no-notifications-sent-when-updates-are-available-D8_1.patch
Git reports that the patch is applied cleanly and I verify that the 3 files are adapted as intended.
Back to /admin/config/system/cron and press button “run cron”
The result is:
Fatal error: Call to undefined function _update_cron_notify() in C:\Users\luk\Desktop\D8\drupal\core\modules\update\update.module on line 307
After undoing the patch ($git reset –hard) cron behaves normally again
Comment #34
akamaus CreditAttribution: akamaus commented@luk.stoops
Looks like update.fetch.inc wasn't loaded for some reason. Or better to say it was loaded for some reason on my test site. Please, try this one.
Comment #35
luk.stoops CreditAttribution: luk.stoops commentedYes, this solved the problem. I added an outdated D8 module to the D8 environment and after the next cron run I received the notification email.
I also back ported the patch to D7 and here too everything works as expected.
I expect to get an email every day now if I set “check for updates” daily and every week if I set “check for updates” weekly.
As I told you php is new for me but if I look at the update_cron () function:
I suppose that $frequency becomes either 1 or 7 depending on the choice daily or weekly and that $interval becomes the number of corresponding seconds.
What surprises me is that the configured update interval and the configured time between notifications both relate to $interval
I understood from the comments in #21 that that you like to get reminders once a week, not more often. So I was expecting on update.module line 304
if ((REQUEST_TIME - variable_get('update_last_email_notification', 0)) > 60 * 60 * 24 * 7) or something like that.
Anyway I’ll keep on testing and let you know if I encounter strange or unexpected results.
Comment #36
catchLatest patch looks good to me, moving back to CNR so the bot tests it.
I've also opened #1566662: Update module should send notifications on configured day to discuss changing this handling to use days of the week rather than just interval.
Comment #37
luk.stoops CreditAttribution: luk.stoops commentedI have a small question about patch #34: If at line 307 it is necessary to add module_load_include('inc', 'update', 'update.fetch'); before _update_cron_notify();
Is it not needed as well before the _update_cron_notify(); at line 297 ?
Comment #38
rjacobs CreditAttribution: rjacobs commentedI just wanted to note that there is a D6 version of a similar issue being discussed at:
#359171: Avoid cases where update data is refreshed on admin pageloads and ensure update notifications are always sent as configured
I'm hesitant to mark that issue as a duplicate of this one for a couple of reasons:
Please let me know if I'm off base with this or if I should move that D6 patch to this queue.
Comment #39
akamaus CreditAttribution: akamaus commented@luk.stoops
no, the call to _update_cron_notify() on 297 is preceeded by call to update_refresh which by itself loads update.fetch.inc. I'm hesitating though, should we refactor the loading and calling _update_cron_notify in a separate function. Like it's the case with for example update_create_fetch_task.
Comment #40
luk.stoops CreditAttribution: luk.stoops commentedAll tests on #34 provided expected results.
I’ll suggest that possible refactoring’s could taken in account in the new spin off issue #1566662 ?
I include the D7 backport.
It’s my first patch ever so please handle with care;-)
What are the next steps to get these committed?
Comment #41
luk.stoops CreditAttribution: luk.stoops commentedtemporally changed version to get the D7 patch bot tested
Comment #42
luk.stoops CreditAttribution: luk.stoops commentedComment #43
djmolny CreditAttribution: djmolny commentedThanks to all who have taken the time to work this problem!
Question: Is there a combination of settings (cron frequency, Available Update, other?) under which notifications *will* be sent?
Comment #44
akamaus CreditAttribution: akamaus commentedIf I remember correctly, the bug triggers if you run cron more often than update check frequency.
Comment #45
akamaus CreditAttribution: akamaus commentedI applied patch #41 on one of my D7 sites and after running cron got a message about pending security updates. So works for me.
Comment #46
Dries CreditAttribution: Dries commentedI think this patch needs a re-roll. Asking for a re-test.
I'm also wondering if we need to move the existing
_update_cron_notify()
out of the if-statement and do the new check outside of the else-statement.Comment #47
Dries CreditAttribution: Dries commented#41: 1263040-no-notifications-sent-when-updates-are-available-40-D7_x.patch queued for re-testing.
Comment #49
catchLooks like Dries re-tested the D7 patch, re-testing the 8.x one this time.
#34: 1263040-no-notifications-sent-when-updates-are-available-D8_2.patch queued for re-testing.
Comment #50
catchDries is right about the original call to _update_cron_notify(), we can just skip that call - variables are tracked separately so can be checked separately too.
Comment #51
luk.stoops CreditAttribution: luk.stoops commentedI tested patch #50 a few days now with the outdated module xbbcode-8.x-1.2 installed.
Got two messages about new updates available. So works as expected.
Comment #52
catchMoving this back to RTBC, was just removing duplicate code.
Comment #53
rjacobs CreditAttribution: rjacobs commentedJust a note that I believe the same general strategy and solution outlined here (now RTBC) is also captured in the D6 patch at #359171: Avoid cases where update data is refreshed on admin pageloads and ensure update notifications are always sent as configured (which attempts to address this same problem, along with one other related issue that is D6 specific). I'm just hoping that will be able to be reviewed as well.
Comment #54
catchCommitted/pushed to 8.x, thanks!
Comment #55
luk.stoops CreditAttribution: luk.stoops commentedBackport to D7
Comment #56
akamaus CreditAttribution: akamaus commentedImmediately got email notification after patching an outdated D7 site with #55 and running drush cron. It was silent for months.
Comment #57
webchickThis looks to be a pretty straight-forward patch, and I don't think it will break anything. Solves what is definitely a major bug.
Committed and pushed to 7.x. Thanks! Probably something worth mentioning in the release notes.
Comment #58
webchickAlso added this to the CHANGELOG at http://drupalcode.org/project/drupal.git/blobdiff/b1b0c57f5c8293e2dd38f3...
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commentedOops, I started reviewing this issue about a week ago, but then life intervened before I actually left a comment here....
I think the patch looks good too and would have committed it as is also, but I had a couple thoughts:
For example, one thing that's a little odd about introducing the second variable is that it seems likely for the variables to get out of sync (i.e, your site will learn about about a critical security issue on Wednesday, but you won't actually get an email about it until the next Monday). That seems contrary to what the code in update_cron() was trying to do, which was basically to notify people as soon as the info is available, per this code comment:
Of course, notifying people a few days late is better than not notifying them at all, so in most cases this patch probably constituted an improvement.... and therefore seems OK at least as an interim step.
Comment #60
dwwThanks for pinging me about this. Sadly, I've been fighting a nasty cold for close to a week now, and am therefore desperately behind on d.o D7 porting work, and I was supposed to leave town this morning for 2.5 weeks. Ugh. :( So, I'm not going to be able to review this until late July or early August, if at all. I'm a terribly update manager maintainer, I know. Woe is me. Anyway, unassigning so no one is blocked on a real review. Sounds like this is in good hands with David's latest comments, at least. ;)
Sorry!
-Derek
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedNo worries - feel better :)
Comment #62
catchDrupal 6 sites still don't get e-mails for update notifications, adding tag and moving back to major.
Comment #63
rjacobs CreditAttribution: rjacobs commentedIn regard to #62 please reference #359171: Avoid cases where update data is refreshed on admin pageloads and ensure update notifications are always sent as configured. Should I move the patch from that issue here and mark #359171 as a dup?
Comment #64
akamaus CreditAttribution: akamaus commented@catch, please look at #5 vs #2. The motivation for proposed solution was the difference in update & report logic in D6 vs D7. The problem I told about in #2 doesn't exist in D6, I don't understand how this patch can be backported.
I regularly receive update notifications from D6 sites I'm looking after. If there is a problem with that though, I believe it must be somewhere else.
Comment #65
rjacobs CreditAttribution: rjacobs commented@akamaus, I believe that the steps to replicate the problem (update mails being missed) is different in D6, but the fix (adding a new variable to track notifications being sent separately from updates being checked) is the same. The missed update notification issue is less common in D6, but it can still happen, and this same general fix appears to address it.
I have not looked into the specifics in some time, but my comment #5 in #359171: Avoid cases where update data is refreshed on admin pageloads and ensure update notifications are always sent as configured goes into some detail. In addition, comment #7 in that same issue notes the connections to this D7 issue.
So maybe the term "backport" is not 100% accurate, but this seems to be a case where this solution can be re-used in a D6 to solve a related problem.
That's my take on this, though I can't speak for catch's observations in D6.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedAfter updating from 7.14 to 7.15 I still have
Call to undefined function _update_cron_notify() in /usr/share/drupal7/modules/update/update.module on line 308
on pages on my website.
Comment #67
tim.plunkettDrupal has a backport policy.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commented#62 moved this to major based on Drupal 6, but comments above pointed out there's already a separate issue for Drupal 6 (and it's major too). We don't need two major issues, especially when one is filed against Drupal 8 and counting against thresholds even though the bug is already fixed :)
Comment #69
David_Rothstein CreditAttribution: David_Rothstein commentedSorry for the noise; I'm told that slashes in tags can break the autocomplete on drupal.org.
Comment #70
delzhand CreditAttribution: delzhand commentedComment #71
botrisGoing to update issue.
Comment #72
botrisUpdated summary & tags.
Comment #86
quietone CreditAttribution: quietone as a volunteer commentedIn #54 this was committed to Drupal 8, Jun 2012, b49c71c.
The backport to D7 was committed in Jul 2012. b1b0c57. It is listed on the release notes, https://www.drupal.org/project/drupal/releases/7.15
Why is this still open? Skimming the issue I suspect this was left open for a backport to D6.
I think it is safe to close this as fixed.