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

[Christian Kuhn]

Hey.

On 12/17/2010 01:06 PM, François Suter wrote:
> 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 ;-)

Thanks :)


>> - * 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?

This variable only holds the already normalized sections, so 1-7 is 
correct at this point. I've added a better documentation to the 
constructor parameter now.


>> + * @param integer $tstamp: optional start time, used in unit tests
>
> $tstamp => $timestamp

Done.


>
>> + * @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

Done.


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

Done.


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

Done.


>> + $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.

I've now changed most of the occurrences in both classes.


>> + * @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

Done.


>> + $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.

Done.


>> + * 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

Done.


>> + * 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)

Done.


>> + /**
>> + * @return array
>> + */
>> + public static function normalizeMonthFieldValidDataProvider() {
>
> The name should be changed, as you're testing also days of week.

Done.


> (cf. IRC discussion about steps interpretation)

Fixed, adapted tests.


>> normalizeMonthFieldReturnsNormalizedListForValidExpression($expression, $isMonthField,
>> $expected) {
>
> Ditto for the name.

Done.


>> + public static function validStepsDataProvider() {
>
> 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).

Adapted some tests to show this.


>> + if (!(count($fields) === 5)) {
>
> count($fields) !== 5

Done.


>> + public static function normalizeMonthAndWeekdayField($expression,
>
> Your code accepts more than three letter codes actually ;-)

Yes, I'm aware of that. I was using the DateTime object in the first 
place which is more specific at this point, but some methods are not 
available in php 5.2, therefore I switched to strtotime. I didn't 
consider this case important enough to catch this (would be easy: if 
strlen > 3 -> throw).


> Again, thanks for this beautiful work.

Thanks for your review!


Regards
Christian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 16611_03.diff
Type: text/x-patch
Size: 66551 bytes
Desc: not available
URL: <http://lists.typo3.org/pipermail/typo3-team-core/attachments/20101220/c2115d31/attachment-0001.bin>