[TYPO3-core] RFC #16611: Bug: [scheduler] tx_scheduler_CronCmd needs refactoring

François Suter fsu-lists at cobweb.ch
Fri Dec 17 13:06:30 CET 2010


Hi Christian,

As promised here's my full review (by reading) of your patch, on top of 
the steps issue discussed via IRC. I have put the issues I have found 
"inside" the patch's code. It may be a bit chaotic, but you should be 
able to find all the places ;-). Anyway it's all really minor compared 
to the steps problem.

In general it's extremely clean code, well documented and covered by an 
impressive number of unit tests. Thanks for having done such a careful 
work. And I've learned quite a bit more stuff regarding unit tests ;-)

> -	 *	field          allowed values
> +	 *	field          lower and upper bound
>   	 *	-----          --------------
>   	 *	minute         0-59
>   	 *	hour           0-23
>   	 *	day of month   1-31
> -	 *	month          1-12 (or names, see below)
> -	 *	day of week    0-7 (0 or 7 is Sun, or use names)
> +	 *	month          1-12
> +	 *	day of week    1-7

Why do you remove the mention of names (or 7 for Sunday) this this new 
version will support all of that?

> -	/**
>   	 * Constructor
>   	 *
> -	 * @param	string		$cmd: the cron command
> -	 * @param	integer		$tstamp: optional start time
> -	 * @return	void
> +	 * @api
> +	 * @param string $cronCommand: the cron command
> +	 * @param integer $tstamp: optional start time, used in unit tests

$tstamp => $timestamp

>   	/**
> -	 * Calulates the next execution
> +	 * Calulates the date of the next execution
>   	 *
> -	 * @param	integer		$level: number of the current level, e.g. 2 is the day level
> -	 * @return	void
> +	 * @api
> +	 * @param integer $level Deprecated number of the current level, e.g. 2 is the day level

Is there any convention for writing "Deprecated" in that place? It just 
feels weird here, because it read like a single sentence. Maybe 
something like:

* @param integer $level (Deprecated) Number of the current level, e.g. 2 
is the day level

> +				// A date must match within the next 4 years, this hight number makes

hight?

> +				// sure leap year cron command configuration are catched.

catched => caught

> +		$isDayOfMonthRestricted = (strcmp($this->cronCommandSections[2], '*') === 0) ? FALSE : TRUE;
> +		$isDayOfWeekRestricted = (strcmp($this->cronCommandSections[4], '*') === 0) ? FALSE : TRUE;

Why not simply use:

$this->cronCommandSections[2] === '*'

I don't think we need binary safety here, do we? strcmp() comparisons 
are just so unintuitive, I like to avoid them as much as possible (of 
course, it's a question of habit).

Same applies to other uses of strcmp() throughout this patch.

>   	/**
> -	 * Builds a list of possible values from a cron command.
> +	 * Determine if a given number validates a cron command section. The given cron
> +	 * command must be a 'normalized' list with only comma separated integers or '*'
>   	 *
> -	 * @param	string		$definition: the command e.g. "2-8,14,0-59/20"
> -	 * @param	integer		$min: minimum allowed value, greater or equal zero
> -	 * @param	integer		$max: maximum allowed value, greater than $min
> -	 * @return	array		list with possible values
> +	 * @param string cron command
> +	 * @param integer number to look up
> +	 * @return boolean TRUE if nuber is in list

Parameter names got lost in the @param

> +			$inList = TRUE;
> +		} else {
> +			$inList = in_array((string)$numberToMatch, explode(',', $commandExpression));

Why not use t3lib_div::inList()? It might be a bit less clean 
conceptually, but it should work in this case.

>   	/**
> -	 * Builds a list of possible values from a single part of a cron command.
> -	 * Parses asterisk (*), ranges (2-4) and steps (2-10/2).
> +	 * Helper method to calculate number of seconds in a day.
>   	 *
> -	 * @param	string		$definition: a command part e.g. "2-8", "*", "0-59/20"
> -	 * @param	integer		$min: minimum allowed value, greater or equal zero
> -	 * @param	integer		$max: maximum allowed value, greater than $min
> -	 * @return	array		list with possible values or empty array
> +	 * This is not always 86400 (60*60*24) and depends on the timezone:
> +	 * Some courtries like germany have a summertime / wintertime switch,

courtries => countries
germany => Germany

> +	 * on every last sunday in march clocks are forwarded by one hour (set from 2:00 to 3:00),
> +	 * and on last sunday of october they are set back one hour (from 3:00 to 2:00).
> +	 * This shortens and lengthens the length of a day by one hour.

In general all days and months start with uppercase in English.

> +			'once every day in februar' =>  array(

februar => february (happens several times)

> +	/**
> +	 * @return array
> +	 */
> +	public static function normalizeMonthFieldValidDataProvider() {

The name should be changed, as you're testing also days of week.

> +		return array(
> +			'*' =>  array('*', TRUE, '*'),
> +			'string 1' =>  array('1', TRUE, '1'),
> +			'jan' =>  array('jan', TRUE, '1'),
> +			'feb/2' =>  array('feb/2', TRUE, '2'),
> +			'jan-mar/2' =>  array('jan-mar/2', TRUE, '2'),
> +			'1-2' =>  array('1-2', TRUE, '1,2'),
> +			'1-3/2,feb,may,6' =>  array('1-3/2,feb,may,6', TRUE, '2,5,6'),
> +			'*/4' =>  array('*/4', TRUE, '4,8,12'),
> +			'*' =>  array('*', FALSE, '*'),
> +			'string 1' =>  array('1', FALSE, '1'),
> +			'fri' =>  array('fri', FALSE, '5'),
> +			'sun' =>  array('sun', FALSE, '7'),
> +			'string 0 for sunday' =>  array('0', FALSE, '7'),
> +			'0,1' =>  array('0,1', FALSE, '1,7'),
> +			'*/3' =>  array('*/3', FALSE, '3,6'),
> +			'tue/2' =>  array('tue/2', FALSE, '2'),
> +			'1-2' =>  array('1-2', FALSE, '1,2'),
> +			'tue-fri/2' =>  array('tue-fri/2', FALSE, '2,4'),
> +			'1-3/2,tue,fri,6' =>  array('1-3/2,tue,fri,6', FALSE, '2,5,6'),
> +		);
> +	}

(cf. IRC discussion about steps interpretation)

> +
> +	/**
> +	 * @test
> +	 * @dataProvider normalizeMonthFieldValidDataProvider
> +	 */
> +	public function normalizeMonthFieldReturnsNormalizedListForValidExpression($expression, $isMonthField, $expected) {

Ditto for the name.

> +	/**
> +	 * @return array
> +	 */
> +	public static function validStepsDataProvider() {
> +		return array(
> +			'2/2' =>  array('2/2', '2'),
> +			'2,3,4/2' =>  array('2,3,4/2', '2,4'),
> +		);
> +	}

I would use more "awkward" steps that better exemplify how steps works. 
I think "/2" is somehow "too simple". What about something like:

1,5,6,8,12/3

The result would be 1,8. And:

7/2

which should result in 7 (again cf. our IRC discussion about steps).

> +	public static function splitFields($cronCommand) {
> +		$fields = explode(' ', $cronCommand);
> +
> +		if (!(count($fields) === 5)) {

Isn't:

count($fields) !== 5

more intuitive?

> +	/**
> +	 * Normalize month field.
> +	 *
> +	 * @param string $expression Month field expression
> +	 * @param boolean $isMonthField TRUE if month field is handled, false for weekday field
> +	 * @return string Normalize expression
> +	 */
> +	public static function normalizeMonthAndWeekdayField($expression, $isMonthField = TRUE) {
> +		if (strcmp($expression, '*') === 0) {
> +			$fieldValues = '*';
> +		} else {
> +				// Fragment espression by , / and - and substitute three letter code of month and weekday to numbers

Your code accepts more than three letter codes actually ;-)


Again, thanks for this beautiful work.

-- 

Francois Suter
Cobweb Development Sarl - http://www.cobweb.ch


More information about the TYPO3-team-core mailing list