Merge lp:~edb/quam-plures/cronjob_stuff into lp:quam-plures

[EdB]

http://forums.quamplures.net/viewtopic.php?f=11&t=739

[lp:~edb/quam-plures/cronjob_stuff]

7604. By EdB on 2011-06-24

adding dracones branch

7605. By EdB on 2011-06-25

clean up posting and validating XHTML

[Tilman Blumenbach (tblue)]

Looks fine, but why do you set $ctst_error to 0 (used to be 1) when setting the default filters for the cronjob list? This way, failed cronjobs are hidden by default; shouldn't the admin see them right away?

[EdB (edb)]

hmmm... dunno why. Don't recall it cuz it's been a while, and for today my mission seems to be somehow fixing this POS software known as bazaar. It decided to always throw an error at me so I'm back to deleting everything and guessing at which version and combination of whatever actually works.

Oh well. Let it sit I guess.

Maybe because it is one tiny little step in a HUGE pile of crap in the core? I mean, there are very few aspects of the whole cronjob arrangement that actually work and this was just a teeny tiny little step towards making the whole thing work. Dunno though. Don't recall and don't have time to play with one little branch right now. Or the ability to play with it given that bazaar decided to die.

[Tilman Blumenbach (tblue)]

Hm, I don't know, it's not like it breaks the branch or something... I can merge it if you think it'd be a good idea? Or if you want me to change it back to 0, just move branch ownership to quam-plures-core (same for the other branches you have in review -- so I can fix stuff if necessary).

[Tilman Blumenbach (tblue)]

Marking as Needs Fixing, see above:

> Looks fine, but why do you set $ctst_error to 0 (used to be 1) when setting
> the default filters for the cronjob list? This way, failed cronjobs are hidden
> by default; shouldn't the admin see them right away?

review: Needs Fixing

[lp:~edb/quam-plures/cronjob_stuff]

7606. By EdB on 2011-07-12

adding core updates

[EdB (edb)]

Setting that thing to 0 may have been a brain-fart, but might have had "hand in the air gives up in disgust" awareness behind it, so bear with me as I explain what might have been the case and before I dig in to code to confirm or deny what I am about to document.

The problem MIGHT be that the error conditions it reports on having nothing to do with cron job errors. Rather, it reports "a problem exists" when the actual problem really is that our code is disgracefully sloppy and fails to clean up after itself. Specifically, if you remove a plugin that was the target of a scheduled job then you get an error. Not because cron failed in any way, and not because cron did a job and returned a failure notification, but because our code failed to remove the cron job from the scheduler when the installation owner instructed the core to remove the plugin.

Additionally and possibly related, I am pretty sure I learned (while trying to understand cron job stuff) that any error code a job returns is ONLY important internally. In other words if a cron job returns 100 it is up to us to interpret that as either good or bad or whatever. And we, of course, have nothing for a plugin author to reference on that topic. Why? Because we inherited a lump of coal that we have not yet compressed and polished into a rather awesome diamond. Again. Thus, all "error reporting" is currently bogus and useless and 100% misleading. Thus, I figured why should we bother to report them?

I think. Just thinking before diving it. Seriously. I could be completely off and it could have just been a complete mistake. I've done that once or twice ;)

Anyway now I'll jump into the code now that my head is a little bit back where it was when I started playing this branch.

[lp:~edb/quam-plures/cronjob_stuff]

7607. By EdB on 2011-07-15

committing core commits :)

[Tilman Blumenbach (tblue)]

Okay, have fun digging. ;)

[lp:~edb/quam-plures/cronjob_stuff]

7608. By EdB on 2011-07-15

restored setting for displaying errors to it's original '1' value

7609. By EdB on 2011-07-15

core up, core in

[EdB (edb)]

Resubmitting after just setting it back to a '1' value.

For the record, this is one very small step down the crontask path that is kinda documented at http://forums.quamplures.net/viewtopic.php?f=11&t=739 Basically what we do here is get rid of 2 "test" jobs that were files and move them into the "test" plugin. Seemed like a no-brainer task to get the ball rolling.

[EdB (edb)]

dang. forgot to select resubmit.

review: Needs Resubmitting

[Tilman Blumenbach (tblue)]

Hmm, let's finally approve this (again).

review: Approve

[lp:~edb/quam-plures/cronjob_stuff]

7610. By EdB on 2011-08-25

core to 7614

7611. By EdB on 2011-08-26

core up to 7618

[EdB (edb)]

Bump? I guess I don't mind doing my own merging as long as no one who is going to look has found any objection. Tblue gave this an "approve", so Lee are you interested in taking a look? If not no problem. If so give a comment so we know, then one of us will know if we should do a merge or not.

I actually do want to avoid self-merges due to it is a bad place to be. Some of my branches "work without errors" but they weren't the best possible code. So the feedback makes it "work without errors and be better code", and self-merging takes away that opportunity.

But look how old this stuff gets :(

[lp:~edb/quam-plures/cronjob_stuff]

7612. By EdB on 2011-09-01

core to 7620

=== modified file 'qp_inc/cron/cronjobs.ctrl.php'
--- qp_inc/cron/cronjobs.ctrl.php	2010-12-31 12:12:03 +0000
+++ qp_inc/cron/cronjobs.ctrl.php	2011-09-01 02:17:27 +0000
@@ -44,72 +44,55 @@
 param( 'results_crontab_order', 'string', '-A', true );
 param( 'results_crontab_page', 'integer', 1, true );
 
-
-// fp> The if  below was the point where THE LINE WAS CROSSED!
-// This is bloated here. This has to go into the action handling block (and maybe a function)
-// THIS IS NO LONGER CONTROLLER INITIALIZATION. THIS IS ACTION EXECUTION!
-// dh> ok. Moved the other param inits above. Ok? I don't think it should be an extra function..
-
 // Init names and params for "static" available jobs and ask Plugins about their jobs:
 if( $action == 'new' || $action == 'create' )
 {
 	// NOTE: keys starting with "plugin_" are reserved for jobs provided by Plugins
 	$cron_job_names = array(
-			'test' => T_('Basic test job'),
-			'error' => T_('Error test job'),
-			'prune_hits_sessions' => T_('Prune old hits & sessions'),
-			// post notifications, not user schedulable
-		);
+		'prune_hits_sessions' => T_('Prune old hits & sessions'),
+	);
 	$cron_job_params = array(
-			'test' => array(
-				'ctrl' => 'cron/jobs/_test.job.php',
-				'params' => NULL ),
-			'error' => array(
-				'ctrl' => 'cron/jobs/_error_test.job.php',
-				'params' => NULL ),
-			'prune_hits_sessions' => array(
-				'ctrl' => 'cron/jobs/_prune_hits_sessions.job.php',
-				'params' => NULL ),
-			// post notifications, not user schedulable
-		);
+		'prune_hits_sessions' => array(
+			'ctrl' => 'cron/jobs/_prune_hits_sessions.job.php',
+			'params' => NULL ),
+	);
 
 	// Get additional jobs from Plugins:
 	foreach( $Plugins->trigger_collect( 'GetCronJobs' ) as $plug_ID => $jobs )
 	{
-		if( ! is_array($jobs) )
+		if( ! is_array( $jobs ) )
 		{
-			$Debuglog->add( sprintf('GetCronJobs() for plugin #%d did not return array. Ignoring its jobs.', $plug_ID), array('plugins', 'error') );
+			$Debuglog->add( sprintf( T_('GetCronJobs() for plugin #%d did not return array. Ignoring its jobs.'), $plug_ID ), array('plugins', 'error') );
 			continue;
 		}
 		foreach( $jobs as $job )
 		{
 			// Validate params from plugin:
-			if( ! isset($job['params']) )
+			if( ! isset( $job['params'] ) )
 			{
 				$job['params'] = NULL;
 			}
-			if( ! is_array($job) || ! isset($job['ctrl'], $job['name']) )
+			if( ! is_array( $job ) || ! isset( $job['ctrl'], $job['name'] ) )
 			{
-				$Debuglog->add( sprintf('GetCronJobs() for plugin #%d did return invalid job. Ignoring.', $plug_ID), array('plugins', 'error') );
+				$Debuglog->add( sprintf( T_('GetCronJobs() for plugin #%d returned an invalid job. Ignoring.'), $plug_ID ), array('plugins', 'error') );
 				continue;
 			}
 			if( isset($job['params']) && ! is_array($job['params']) )
 			{
-				$Debuglog->add( sprintf('GetCronJobs() for plugin #%d did return invalid job params (not an array). Ignoring.', $plug_ID), array('plugins', 'error') );
+				$Debuglog->add( sprintf( T_('GetCronJobs() for plugin #%d did return invalid job params (not an array). Ignoring.'), $plug_ID ), array('plugins', 'error') );
 				continue;
 			}
 			$ctrl_id = 'plugin_'.$plug_ID.'_'.$job['ctrl'];
 
 			$cron_job_names[$ctrl_id] = $job['name'];
 			$cron_job_params[$ctrl_id] = array(
-					'ctrl' => $ctrl_id,
-					'params' => $job['params'],
-				);
+				'ctrl' => $ctrl_id,
+				'params' => $job['params'],
+			);
 		}
 	}
 }
 
-
 switch( $action )
 {
 	case 'new':
@@ -157,7 +140,7 @@
 		$edited_Cronjob->set( 'params', $cron_job_params[$cjob_type]['params'] );
 
 		if( ! param_errors_detected() )
-		{	// No errors
+		{ // No errors
 
 			// Save to DB:
 			$edited_Cronjob->dbinsert();
@@ -178,7 +161,7 @@
 		// TODO: prevent deletion of running tasks.
 		$DB->begin();
 
-		$tsk_status =	$DB->get_var(
+		$tsk_status = $DB->get_var(
 			'SELECT clog_status
 				 FROM T_cron__log
 				WHERE clog_ctsk_ID= '.$ctsk_ID,
@@ -187,17 +170,18 @@
 		if( $tsk_status == 'started' )
 		{
 			$DB->rollback();
-
 			$Messages->add(  sprintf( T_('Job #%d is currently running. It cannot be deleted.'), $ctsk_ID ), 'error' );
 		}
 		else
 		{
 			// Delete task:
-			$DB->query( 'DELETE FROM T_cron__task
+			$DB->query( 'DELETE
+										 FROM T_cron__task
 										WHERE ctsk_ID = '.$ctsk_ID );
 
 			// Delete log (if exists):
-			$DB->query( 'DELETE FROM T_cron__log
+			$DB->query( 'DELETE
+										 FROM T_cron__log
 										WHERE clog_ctsk_ID = '.$ctsk_ID );
 
 			$DB->commit();
@@ -209,12 +193,12 @@
 		$action = 'list';
 		break;
 
-
 	case 'view':
 		$cjob_ID = param( 'cjob_ID', 'integer', true );
 
 		$sql =  'SELECT *
-							 FROM T_cron__task LEFT JOIN T_cron__log ON ctsk_ID = clog_ctsk_ID
+							 FROM T_cron__task
+					LEFT JOIN T_cron__log ON ctsk_ID = clog_ctsk_ID
 							WHERE ctsk_ID = '.$cjob_ID;
 		$cjob_row = $DB->get_row( $sql, OBJECT, 0, 'Get cron job and log' );
 		if( empty( $cjob_row ) )
@@ -226,16 +210,14 @@
 
 	case 'list':
 		// Detect timed out tasks:
-		$sql = " UPDATE T_cron__log
-								SET clog_status = 'timeout'
-							WHERE clog_status = 'started'
-										AND clog_realstart_datetime < ".$DB->quote( date2mysql( time() + $time_difference - $cron_timeout_delay ) );
+		$sql = "UPDATE T_cron__log
+							 SET clog_status = 'timeout'
+						 WHERE clog_status = 'started'
+							 AND clog_realstart_datetime < ".$DB->quote( date2mysql( time() + $time_difference - $cron_timeout_delay ) );
 		$DB->query( $sql, 'Detect cron timeouts.' );
-
 		break;
 }
 
-
 // Display <html><head>...</head> section! (Note: should be done early if actions do not redirect)
 $AdminUI->disp_html_head();
 
@@ -269,4 +251,4 @@
 // Display body bottom, debug info and close </html>:
 $AdminUI->disp_global_footer();
 
-?>
\ No newline at end of file
+?>

=== removed file 'qp_inc/cron/jobs/_error_test.job.php'
--- qp_inc/cron/jobs/_error_test.job.php	2009-12-12 16:12:01 +0000
+++ qp_inc/cron/jobs/_error_test.job.php	1970-01-01 00:00:00 +0000
@@ -1,15 +0,0 @@
-<?php
-/**
- * This file implements the Error Test Cron controller
- *
- * @author fplanque: Francois PLANQUE
- *
- * @package pond
- */
-if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );
-
-$result_message = T_('The Error TEST cron controller simulates an error, thus this "error" is normal!');
-
-return 100; /* Simulated error */
-
-?>
\ No newline at end of file

=== modified file 'qp_inc/cron/jobs/_post_notifications.job.php'
--- qp_inc/cron/jobs/_post_notifications.job.php	2009-12-12 16:12:01 +0000
+++ qp_inc/cron/jobs/_post_notifications.job.php	2011-09-01 02:17:27 +0000
@@ -4,16 +4,12 @@
  *
  * @author fplanque: Francois PLANQUE
  *
- * @todo dh> Should this also handle feedback notifications (according to the "outbound_notifications_mode" setting)?
- * fp> No. The feedback notifications should have their own job.
- *
  * @package pond
  */
 if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );
 
 global $Settings;
 
-
 if( $Settings->get( 'outbound_notifications_mode' ) != 'cron' )
 { // Autopruning is NOT requested
 	$result_message = T_('Post notifications are not set to run as a scheduled task.');
@@ -23,30 +19,26 @@
 // Get the ID of the post we are supposed to post-process:
 if( empty( $job_params['item_ID'] ) )
 {
-	$result_message = 'No item_ID parameter received.'; // No trans.
+	$result_message = T_('No item_ID parameter received.');
 	return 3;
 }
 
 $item_ID = $job_params['item_ID'];
 
-
 // Notify that we are going to take care of that post's post processing:
 $DB->query( 'UPDATE T_items__item
 								SET post_notifications_status = "started"
 							WHERE post_ID = '.$item_ID.'
-							  AND post_notifications_status = "todo"
-							  AND post_notifications_ctsk_ID = '.$job_params['ctsk_ID'] );
+								AND post_notifications_status = "todo"
+								AND post_notifications_ctsk_ID = '.$job_params['ctsk_ID'] );
 if( $DB->rows_affected != 1 )
-{	// We would not "lock" the requested post
+{ // We would not "lock" the requested post
 	$result_message = sprintf( T_('Could not lock post #%d. It may already be processed.'), $item_ID );
 	return 4;
 }
 
 // Get the Item:
 $ItemCache = & get_Cache( 'ItemCache' );
-/**
- * @var Item
- */
 $edited_Item = & $ItemCache->get_by_ID( $item_ID );
 
 // send outbound pings:
@@ -65,4 +57,4 @@
 
 return 1; /* ok */
 
-?>
\ No newline at end of file
+?>

=== removed file 'qp_inc/cron/jobs/_test.job.php'
--- qp_inc/cron/jobs/_test.job.php	2009-12-12 16:12:01 +0000
+++ qp_inc/cron/jobs/_test.job.php	1970-01-01 00:00:00 +0000
@@ -1,15 +0,0 @@
-<?php
-/**
- * This file implements the test Cron controller
- *
- * @author fplanque: Francois PLANQUE
- *
- * @package pond
- */
-if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );
-
-$result_message = T_('The TEST cron controller says hello!');
-
-return 1; /* ok */
-
-?>
\ No newline at end of file

=== modified file 'qp_inc/cron/views/_cronjob_list.view.php'
--- qp_inc/cron/views/_cronjob_list.view.php	2011-03-07 14:26:49 +0000
+++ qp_inc/cron/views/_cronjob_list.view.php	2011-09-01 02:17:27 +0000
@@ -24,7 +24,6 @@
  */
 if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );
 
-
 // Get filters:
 global $ctst_pending, $ctst_started, $ctst_timeout, $ctst_error, $ctst_finished;
 if( !$ctst_pending && !$ctst_started && !$ctst_timeout && !$ctst_error && !$ctst_finished )
@@ -33,6 +32,7 @@
 	$ctst_started = 1;
 	$ctst_timeout = 1;
 	$ctst_error = 1;
+	$ctst_finished = 0;
 }
 
 /*
@@ -67,7 +67,6 @@
 
 $Results->title = get_manual_link( 'scheduled-jobs' ).T_('Scheduled jobs');
 
-
 $Results->global_icon( T_('Refresh'), 'refresh', regenerate_url(), T_('Refresh'), 3, 4 );
 if( $current_User->check_perm( 'options', 'edit', false, NULL ) )
 {	// Permission to edit settings:
@@ -99,41 +98,40 @@
 		)
 	);
 
-
-$Results->cols[] = array(
-						'th' => T_('ID'),
-						'order' => 'ctsk_ID',
-						'th_class' => 'shrinkwrap',
-						'td_class' => 'shrinkwrap',
-						'td' => '$ctsk_ID$'
-					);
-
-$Results->cols[] = array(
-						'th' => T_('Planned at'),
-						'order' => 'ctsk_start_datetime',
-						'td_class' => 'shrinkwrap',
-						'td' => '$ctsk_start_datetime$',
-					);
-
-$Results->cols[] = array(
-						'th' => T_('Name'),
-						'order' => 'ctsk_name',
-						'td' => '<a href="%regenerate_url(\'action,cjob_ID\',\'action=view&amp;cjob_ID=$ctsk_ID$\')%">$ctsk_name$</a>',
-					);
-
-$Results->cols[] = array(
-						'th' => T_('Status'),
-						'order' => 'status',
-						'td_class' => 'shrinkwrap cron_$status$',
-						'td' => '$status$',
-					);
-
-$Results->cols[] = array(
-						'th' => T_('Repeat'),
-						'order' => 'ctsk_repeat_after',
-						'td_class' => 'shrinkwrap',
-						'td' => '$ctsk_repeat_after$',
-					);
+$Results->cols[] = array(
+	'th' => T_('ID'),
+	'order' => 'ctsk_ID',
+	'th_class' => 'shrinkwrap',
+	'td_class' => 'shrinkwrap',
+	'td' => '$ctsk_ID$'
+);
+
+$Results->cols[] = array(
+	'th' => T_('Planned at'),
+	'order' => 'ctsk_start_datetime',
+	'td_class' => 'shrinkwrap',
+	'td' => '$ctsk_start_datetime$',
+);
+
+$Results->cols[] = array(
+	'th' => T_('Name'),
+	'order' => 'ctsk_name',
+	'td' => '<a href="%regenerate_url(\'action,cjob_ID\',\'action=view&amp;cjob_ID=$ctsk_ID$\')%">$ctsk_name$</a>',
+);
+
+$Results->cols[] = array(
+	'th' => T_('Status'),
+	'order' => 'status',
+	'td_class' => 'shrinkwrap cron_$status$',
+	'td' => '$status$',
+);
+
+$Results->cols[] = array(
+	'th' => T_('Repeat'),
+	'order' => 'ctsk_repeat_after',
+	'td_class' => 'shrinkwrap',
+	'td' => '$ctsk_repeat_after$',
+);
 
 function crontab_actions( $ctsk_ID, $status )
 {
@@ -143,26 +141,21 @@
 
 	if( $status != 'started' && $current_User->check_perm( 'options', 'edit', false, NULL ) )
 	{	// User can edit options:
-    $col = action_icon( T_('Delete this job!'), 'delete',
-												regenerate_url( 'action', 'ctsk_ID='.$ctsk_ID.'&amp;action=delete') );
+    $col = action_icon( T_('Delete this job!'), 'delete', regenerate_url( 'action', 'ctsk_ID='.$ctsk_ID.'&amp;action=delete' ) );
 	}
 
 	return $col;
 }
 $Results->cols[] = array(
-					'th' => T_('Actions'),
-					'td_class' => 'shrinkwrap',
-					'td' => '%crontab_actions( #ctsk_ID#, #status# )%',
-				);
-
-
+	'th' => T_('Actions'),
+	'td_class' => 'shrinkwrap',
+	'td' => '%crontab_actions( #ctsk_ID#, #status# )%',
+);
 
 // Display results :
 $Results->display();
 
-
 global $srvc_url;
 echo '<p>[<a href="'.$srvc_url.'cron_exec.php" onclick="return pop_up_window( \''.$srvc_url.'cron_exec.php\', \'evo_cron\' )" target="evo_cron">'.T_('Execute pending jobs in a popup window now!').'</a>]</p>';
 
-
-?>
\ No newline at end of file
+?>

=== modified file 'qp_plugins/test_plugin/README.html'
--- qp_plugins/test_plugin/README.html	2010-12-18 18:52:49 +0000
+++ qp_plugins/test_plugin/README.html	2011-09-01 02:17:27 +0000
@@ -101,6 +101,7 @@
 <h2>Release History:</h2>
 <div id="log">
 <dl>
+<dt>0.2</dt><dd>moved sample cronjobs into this plugin (18-Jun-11)</dd>
 <dt>0.1</dt><dd>initial release (1-Jan-11)</dd>
 </dl>
 </div>

=== modified file 'qp_plugins/test_plugin/_test.plugin.php'
--- qp_plugins/test_plugin/_test.plugin.php	2010-12-31 11:43:06 +0000
+++ qp_plugins/test_plugin/_test.plugin.php	2011-09-01 02:17:27 +0000
@@ -58,7 +58,7 @@
 	var $sub_group = 'plugin demo'; // used if $group is 'other'
 	var $number_of_installs = 1;
 	var $priority = 50;
-	var $version = '0.1';
+	var $version = '0.2';
 
 
 	/**
@@ -234,9 +234,9 @@
 	function GetExtraEvents()
 	{
 		return array(
-				// Gets "min" and "max" as params and should return a random number in between:
-				'test_plugin_get_random' => $this->T_('TEST plugin event that returns a random number.'),
-			);
+			// Gets "min" and "max" as params and should return a random number in between:
+			'test_plugin_get_random' => $this->T_('TEST plugin event that returns a random number.'),
+		);
 	}
 
 
@@ -245,13 +245,19 @@
 	 */
 	function GetCronJobs( & $params )
 	{
-		return array(
-				array(
-					'name' => $this->T_('TEST plugin - cron job'),
-					'ctrl' => 'test_job',
-					'params' => array( 'param' => 1 ),
-				),
-			);
+		$jobs = array(
+			array(
+				'name' => $this->T_('TEST plugin - cron job'),
+				'ctrl' => 'test_job',
+				'params' => array( 'param' => 1 )
+			),
+			array(
+				'name' => $this->T_('TEST plugin - cron job ERROR'),
+				'ctrl' => 'test_job_error',
+				'params' => array( 'param' => 1 )
+			),
+		);
+		return $jobs;
 	}
 
 
@@ -262,7 +268,11 @@
 	{
 		if( $params['ctrl'] == 'test_job' )
 		{
-			return array( 'code' => 1, 'message' => $this->T_('Test successful.') );
+			return array( 'code' => 1, 'message' => $this->T_('The TEST plugin cron job says hello!') );
+		}
+		if( $params['ctrl'] == 'test_job_error' )
+		{
+			return array( 'code' => 100, 'message' => $this->T_('The TEST plugin Error cron job simulates an error, thus this "error" is normal!') );
 		}
 	}