accept more than 2 parameters in CONCAT function #583

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

broncha commented Feb 19, 2013

The DBAL Platform supports more then 2 parameters but the ConcatFunction only validates 2 parameters to CONCAT. This commit allows to pass more than 2 parameters to CONCAT. Also this change would require that getConcatExpression accept array as a parameter. I have opened a pull request for that as well.

Here is the pull request : doctrine/dbal#275

I also propose that the function getConcatExpression only accept array of string rather than multiple string arguments.

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2304

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
+
+ while ($parser->getLexer()->isNextToken(Lexer::T_COMMA)) {
+ $parser->match(Lexer::T_COMMA);
+ $this->concatExpressions[] = $parser->StringPrimary();
@Majkl578

Majkl578 Feb 19, 2013

Member

Wrong indentation by tab.

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
@@ -22,7 +22,7 @@
use Doctrine\ORM\Query\Lexer;
/**
- * "CONCAT" "(" StringPrimary "," StringPrimary ")"
+ * "CONCAT" "(" StringPrimary "," StringPrimary "," StringPrimary ",...)"
@stof

stof Feb 19, 2013

Member

This is not the right syntax for this in EBNF It should be "CONCAT" "(" StringPrimary "," StringPrimary {"," StringPrimary }* ")"

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
+
+ $args = array();
+
+ foreach($this->concatExpressions as $e){
@stof

stof Feb 19, 2013

Member

missing space after foreach. And you should name the variable $expression rather than $e to have a meanignful name

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
- public $secondStringPrimary;
-
+ public $concatExpressions = array();
+
@stof

stof Feb 19, 2013

Member

Please remove all trailing whitespaces. Empty line should be really empty

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
+ $args = array();
+
+ foreach($this->concatExpressions as $e){
+ $args[] = $sqlWalker->walkStringPrimary($this->firstStringPrimary);
@stof

stof Feb 19, 2013

Member

This is broken. $this->firstStringPrimary does not exist anymore

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
- public $firstStringPrimary;
- public $secondStringPrimary;
-
+ public $concatExpressions = array();
@stof

stof Feb 19, 2013

Member

Changing the public properties is a BC break for any code dealing with the AST

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
+
+ $this->concatExpressions[] = $parser->StringPrimary();
+
+ while ($parser->getLexer()->isNextToken(Lexer::T_COMMA)) {
@stof

stof Feb 19, 2013

Member

This will not require to have at least 2 arguments. It is wrong IMO.

Member

stof commented Feb 19, 2013

The new feature should be tested.
And you should run the tests when doing changes (or at least look at the Travis result on the pull request) as they spotted the issue about the wrong property

Contributor

broncha commented Feb 20, 2013

doctrine2/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php is passing given the DBAL Platform has been patched. I doubt if the travis build will pass as the pull request I sent fot DBAL has not been merged

Member

stof commented Feb 20, 2013

If this depend on an unmerged PR in DBAL, please add the link in the description of the PR.

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
+ $args[] = $sqlWalker->walkStringPrimary($expression);
+ }
+
+ return $platform->getConcatExpression( $args );
@stof

stof Feb 20, 2013

Member

you should remove the extra spaces around $args

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php
+
+ while ($parser->getLexer()->isNextToken(Lexer::T_COMMA)) {
+ $parser->match(Lexer::T_COMMA);
+ $this->concatExpressions[] = $parser->StringPrimary();
@stof

stof Feb 20, 2013

Member

wrong indentation

Member

stof commented Feb 20, 2013

you still need to add a unit test (in https://github.com/doctrine/doctrine2/blob/master/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php) for the case where you have more than 2 expressions

Contributor

broncha commented Feb 20, 2013

Unit test SelectSqlGenerationTest::testSupportsMoreThanTwoParametersInConcatFunction added and passing..
Indentation looks fine in eclipse, but seems distorted when I push. I dont know whats happening.

calling getConcatExpression using call_user_func_array as number of a…
…rguments is not known removing dependency to patch DBAL
Contributor

broncha commented Feb 26, 2013

I have pushed a new commit, which does not require doctrine/dbal#275 to be merged.
Travis build should now pass

Owner

beberlei commented Mar 12, 2013

Merged in 4841a06

@beberlei beberlei closed this Mar 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment