[TYPO3-core] RFC #10411: sqlparser is not able to parse more than 1 join

Xavier Perseguers typo3 at perseguers.ch
Wed Oct 28 16:53:07 CET 2009


Hi Oliver,

> - Please put setUp and tearDown as the first two methods in your
> testcase (and the utility method after that) to improve readability.

OK.

> - In tearDown, please unset($this->fixture) to help the garbage collector.

OK.

> - cleanSql expects as string as first parameter (according to the
> PHPDoc). So you don't need to check for is_string in the function, but
> you need to make sure all callers actually provide a string. Some

This is an old definition. It is actually a mixed type because it might be that I get an array. Many SQL parser methods returns an array if all went well and an error message if not. I use this not to 
be forced to declare what is the actual array output of the method. This is a first approximation but is enough I guess for many cases because if I get an array and the parsedString is of the expected 
length after the call, then the string could be parsed.

> - In tests, I find it more readable if the expected value and the actual
> value are on different lines (and the message on another line):
> 
>   $this->assertEquals(
>     'foo',
>     $this->fixture->bar(),
>     'No coffee today ...'
>   );

I used Karsten's method to invoke protected methods and as parsedString needs to be passed by reference, all parameters need to be passed by reference even if it is not needed. I often stored what is 
expected in an $expected variable. As such having $this->assertEquals($expected, $actual); is not less readable. This is more a question of style.

> - If the name of a test functions is descriptive enough, the asserts
> usually don't need a message.

I don't like writing a message unless this makes sense (for assertTrue() for instance).

> - I recommend checking the return type of functions in separate tests.
> For example, a test like this ...:
> 
> +	public function canParseWhereClause() {
> +		$parseString = 'uid IN (1,2) AND (starttime < ' . time() . ' OR
> cruser_id + 10 < 20)';
> +		$where = $this->fixture->parseWhereClause($parseString);
> +
> +		if (!is_array($where)) {
> +			$this->fail($where);
> +		}
> +		$this->assertTrue(empty($parseString), 'parseString is not empty');
> +	}
> 
> 
> ... could sprout a test parseWhereClauseReturnsArray() {
>     ... assertTrue(
>        is_array(...)
>     )}

OK.

> - There's no need to make cleanSql static.

I had the thought that it would be better to make methods static where there was no need to have them non-static but it seems they are a bit slower. OK then.

> - I wonder why cleanSql is needed at all. Care to elaborate on this?

Because the SQL generated by sqlparser contains new lines and additional space characters that make no sense to compare as this. After all we want a valid SQL to be generated, not a given SQL with 
known new line logic, ...

-- 
Xavier Perseguers
http://xavier.perseguers.ch/en

One contribution a day keeps the fork away


More information about the TYPO3-team-core mailing list