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.
drupal_cron_cleanup uses the cron_semaphore
variable which was removed in #805702: Cron should use locking framework .
Comment | File | Size | Author |
---|---|---|---|
#28 | 1775488-D7-drupal_cron_cleanup-28.patch | 931 bytes | mgifford |
#24 | 1775488-D7-drupal_cron_cleanup-24.patch | 998 bytes | mgifford |
#22 | 1775488-D7-drupal_cron_cleanup-22.patch | 1.21 KB | mgifford |
#20 | 1775488-D7-drupal_cron_cleanup-20.patch | 1.02 KB | mgifford |
#16 | 1775488-D7-drupal_cron_cleanup-16.patch | 526 bytes | vijaycs85 |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedSomething like this, perhaps?
Comment #2
julien CreditAttribution: julien commentedthat simple and right i think, but if variable has been removed. will it make sense to remove it also in lock() ?
Comment #3
amateescu CreditAttribution: amateescu commentedThat's a different variable :)
Comment #4
BerdirFix looks good, interesting how long it took to find this :)
I'm wondering if we can add test coverage for this. Maybe add a cron implementation that does exit() which we then call through a php request? (drupal_run_cron() obviously won't work). Then we can check that the lock is gone and we have a watchdog entry...
Comment #5
chx CreditAttribution: chx commentedWe only found this because #1175054: Add a storage (API) for persistent non-configuration state mentioned cron semaphore and when I tried to convert it I found it doesn't exist any more...
Comment #6
alexpottI'm not sure that the shutdown function is even necessary anymore.
Doesn't this mean that after 240 seconds we'll be able to reacquire the lock regardless? And wasn't the issue with the semaphore variable that if cron crashed then the semaphore variable had to be manually reset so that you could run cron again? The shutdown function was solving this and hence, is no longer needed since the lock will be released.
Comment #7
BerdirYes, lock does a releaseAll, so it's quite possible that we don't need this anymore, the only problem would be that we'd loose that watchdog message. But no 7.x site on this world has ever seen this watchdog message, so let's just remove the whole function.
Comment #8
vijaycs85I hope, understand the discussion proper, we are planning to remove cleanup as it is not necessary anymore(because of lock does a reaseAll). If it is true, here is the patch for this cleanup. Also can we rename the issue something like "Removing drupal_cron_cleanup"?
Comment #9
BerdirYes, I think we can remove this. The only thing we lose is that watchdog message, which is misleading anyway, see #308808: Drupal_cron_cleanup paints the wrong picture.
Also marked a bunch of issues as duplicate:
- #302739: Watchdog interferes with cron semaphore deletion in drupal_cron_cleanup()
- #1777162: drupal_cron_cleanup() does nothing due to legacy code
- #942256: Remove drupal_cron_cleanup leftover since we moved to the lock api
Last chance for anyone to disagree :)
Comment #10
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for backport.
Comment #11
vijaycs85Here is 7.x patch.
Comment #12
BerdirBackport looks good. As explain above, this never did anything in 7.x because there is no such variable anymore.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedUgh, that's unfortunate.
For Drupal 7, shouldn't this leave the function itself around though (and mark it as deprecated) rather than completely removing it?
Comment #14
BerdirHm, good point, we could do that. Not sure if we should keep the function as is or replace the function body with just a comment.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedSeems like it would be simpler to just keep the function body in place?
I mean, if someone out there is calling it directly (which is unlikely in the first place), it's possible they're doing something else non-standard such that it actually works - I doubt it, but you never know :)
Comment #16
vijaycs85Updating patch to remove call to
drupal_register_shutdown_function('drupal_cron_cleanup');
Comment #17
BerdirCan we also add a @deprecated to that function and say that it is not used and only kept in place for backwards compatibility?
Comment #18
mgifford@Bedir - so are you thinking of something like:
Comment #19
BerdirNo, that is not what I meant, what I meant is to add the @deprecated to the function that we no longer call, to make it clear that is not doing anything and is just kept to avoid fatal errors if some code out there calls it.
Comment #20
mgiffordCloser?
Comment #21
vijaycs85Can we say bit more like this?
Comment #22
mgiffordI don't think we have a styleguide for this, but I tried to follow what was done in modules/field/field.module for depreciated functions there.
Comment #23
dcam CreditAttribution: dcam commentedFollowing the standard for deprecating functions set by Field seems ok to me, but see @David_Rothstein's concern in #13 and #15 about deleting the function body. Also, none of the Field functions had their code deleted. If we're using that as a standard, then I agree we should leave it.
Comment #24
mgiffordThere seem to be enough of these that a policy might be useful.
Seems like a fairly fringe edge case. Anyways, with the function code left intact as requested.
Comment #25
mgiffordoops
Comment #26
dcam CreditAttribution: dcam commentedI think we can eliminate this line since we're not deleting the function body. Besides, "compatibility" is misspelled.
Looks good otherwise. I think it's nearly ready for RTBC.
Comment #27
dcam CreditAttribution: dcam commentedForgot the status.
Comment #28
mgiffordgood points...
Comment #29
dcam CreditAttribution: dcam commentedOk, I think it's ready to go.
Comment #32
dcam CreditAttribution: dcam commentedComment #35
dcam CreditAttribution: dcam commentedComment #38
dcam CreditAttribution: dcam commentedComment #41
dcam CreditAttribution: dcam commentedComment #42
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
However, the patch was using @see incorrectly and I didn't think the issue it linked to was particularly helpful for understanding why the function is deprecated... so I replaced it with this text on commit instead: