Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
Issue tags: +Needs backport to D7
FileSize
712 bytes

Something like this, perhaps?

julien’s picture

that simple and right i think, but if variable has been removed. will it make sense to remove it also in lock() ?

amateescu’s picture

That's a different variable :)

Berdir’s picture

Fix 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...

chx’s picture

We 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...

alexpott’s picture

I'm not sure that the shutdown function is even necessary anymore.

  // Try to allocate enough time to run all the hook_cron implementations.
  drupal_set_time_limit(240);

  $return = FALSE;
  // Grab the defined cron queues.
  $queues = module_invoke_all('queue_info');
  drupal_alter('queue_info', $queues);

  // Try to acquire cron lock.
  if (!lock()->acquire('cron', 240.0)) {
    // Cron is still running normally.
    watchdog('cron', 'Attempting to re-run cron while it is already running.', array(), WATCHDOG_WARNING);
  }
  else {
    // Run cron... 

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.

Berdir’s picture

Status: Needs review » Needs work

Yes, 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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.08 KB

I 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"?

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, 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 :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

vijaycs85’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.16 KB

Here is 7.x patch.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Backport looks good. As explain above, this never did anything in 7.x because there is no such variable anymore.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Ugh, 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?

Berdir’s picture

Hm, 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.

David_Rothstein’s picture

Seems 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 :)

vijaycs85’s picture

Updating patch to remove call to drupal_register_shutdown_function('drupal_cron_cleanup');

Berdir’s picture

Can we also add a @deprecated to that function and say that it is not used and only kept in place for backwards compatibility?

mgifford’s picture

Issue summary: View changes

@Bedir - so are you thinking of something like:

-    // Register shutdown callback.
-    drupal_register_shutdown_function('drupal_cron_cleanup');
+/*
+ * @depreciated
+ *   The  drupal_register_shutdown_function() deprecated and only kept in 
+  *  place for backwards compatibility.
+ */
Berdir’s picture

No, 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.

mgifford’s picture

Closer?

vijaycs85’s picture

+++ b/includes/common.inc
@@ -5310,17 +5308,9 @@ function drupal_cron_run() {
+ * @deprecated

Can we say bit more like this?


 @deprecated The functionality of this function has been removed as part of https://www.drupal.org/node/805702.
 This function exist to provide backward computability  avoid fatal error on website's calling it directly.

mgifford’s picture

I 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.

dcam’s picture

Status: Needs review » Needs work

Following 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.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
998 bytes

There 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.

mgifford’s picture

Status: Reviewed & tested by the community » Needs review

oops

dcam’s picture

+++ b/includes/common.inc
@@ -5308,10 +5306,11 @@ function drupal_cron_run() {
+ *   This empty function exist to provide backward computability.

I 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.

dcam’s picture

Status: Needs review » Needs work

Forgot the status.

mgifford’s picture

Status: Needs work » Needs review
FileSize
931 bytes

good points...

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I think it's ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 1775488-D7-drupal_cron_cleanup-28.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 1775488-D7-drupal_cron_cleanup-28.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 1775488-D7-drupal_cron_cleanup-28.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 1775488-D7-drupal_cron_cleanup-28.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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:

--- a/includes/common.inc
+++ b/includes/common.inc
@@ -5340,8 +5340,11 @@ function drupal_cron_run() {
 /**
  * DEPRECATED: Shutdown function: Performs cron cleanup.
  *
+ * This function is deprecated because the 'cron_semaphore' variable it
+ * references no longer exists. It is therefore no longer used as a shutdown
+ * function by Drupal core.
+ *
  * @deprecated
- *   @see more details at https://www.drupal.org/node/805702.
  */
 function drupal_cron_cleanup() {
   // See if the semaphore is still locked.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.