[TYPO3-core] RFC: Bug #13194: tx_scheduler_CronCmd does not correctly parse step values

[Oliver Klee]

Hi,

+1 by reading and testing (running the unit tests with and without the
patch) if you change the following things:

> Index: typo3/sysext/scheduler/class.tx_scheduler_croncmd.php
> ===================================================================
>  	protected function getList($definition, $min, $max) {
> +		$resultList = array();

I'd prefer a semantic name that reflects what the variable contains
(instead of the type).

> +			// Explode comma separated parts and handle them independent
> +		$listParts = t3lib_div::trimExplode(',', $definition, TRUE);

The comment is not necessary as the code is "speaking" enough.

> +		foreach ($listParts as $partNumber => $part) {

$partNumber is not used.

> +			$resultList = array_merge($resultList, $this->getListPart($part, $min, $max));
> +		}

You can use + instead of array_merge.

> +
> +			// Sort the list and return it
> +		sort($resultList);
> +		return $resultList;
> +	}

The comment is not necessary as the code is "speaking" enough.

> +	/**
> +	 * Build a list of possible values from a single part of a cron command.

Build -> Builds

> +	 * Parses asterisk (*), ranges (2-4) and steps (2-10/2).
> +	 *
> +	 * @param	string		$definition: a command part e.g. "2-8", "*", "0-59/20"
> +	 * @param	integer		$min: minimum allowed value
> +	 * @param	integer		$max: maximum allowed value
> +	 * @return	array		list with possible values
> +	 */
> +	protected function getListPart($definition, $min, $max) {

Please also document whether $min and $max need to be >= 0, > 0 or
something like that.

Please document whether the return value might also be empty.

>  			}
>  		} else if (strpos($definition, '/') !== false) {
>  				// Get list for step values
> -
> -				// Extract list part
> -			$defList = substr($definition, 0, strpos($definition, '/'));
> -			$stepDef = substr($definition, strpos($definition, '/') + 1);
> -			$tmpList = $this->getList($defList, $min, $max);
> -
> -			for ($i=0; $i<count($tmpList); $i++) {
> -				if ($i % $stepDef == 0) {
> -					$list[] = $tmpList[$i];
> +			list($defList, $stepDef) = t3lib_div::trimExplode('/', $definition);

I don't find the "list" command very easy to read ... is there a way to
write the same code in another way?

> +			$tmpList = $this->getListPart($defList, $min, $max);
> +			foreach ($tmpList as $tmpListNumber => $tmpListValue) {
> +				if ($tmpListValue % $stepDef == 0) {
> +					$list[] = $tmpListValue;
>  				}
>  			}

Please use more meaningful variable names without abbreviations (tmp*,
Def...).

>  	/**
> +	 * Tests wether step values are correctly parsed for minutes
> +	 *
> +	 * @test
> +	 */
> +	public function isMinuteListCorrectForSteps() {
> +		$cronCmdInstance = t3lib_div::makeInstance('tx_scheduler_cronCmd', '0-20/10 * * * *');
> +		$expectedResult = array(
> +			'0' => 0,
> +			'1' => 10,
> +			'2' => 20,
> +		);
> +		$actualResult = $cronCmdInstance->valid_values;
> +		$this->assertEquals($expectedResult, $actualResult[0]);
> +	}

Please use test names that clearly state the condition (or the input)
and the expected result/behavior. "Correctly" is a very general term.
The function name should be so descriptive that no additional function
documentation comment should be needed.


You can inline $expectedResult and $actualResult, if you like (if you
put both on separate lines).


>  	/**
> +	 * Tests whether dayList is correctly calculated for stops of month days
> +	 *
> +	 * @test
> +	 */
> +	public function isDayListCorrectForStepsOfDayOfMonth() {

Ditto.

> +	/**
> +	 * Tests whether dayList is correctly calculated for stops of month days combined with ranges and lists
> +	 *
> +	 * @test
> +	 */
> +	public function isDayListCorrectForStepsOfDayOfMonthCombinedWithLists() {

Ditto.


Oli
-- 
Certified TYPO3 Integrator | TYPO3 Security Team Member