Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accept more than 2 parameters in CONCAT function #583

Closed
wants to merge 5 commits into from
Closed

accept more than 2 parameters in CONCAT function #583

wants to merge 5 commits into from

Conversation

broncha
Copy link
Contributor

@broncha 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.

@doctrinebot
Copy link

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


while ($parser->getLexer()->isNextToken(Lexer::T_COMMA)) {
$parser->match(Lexer::T_COMMA);
$this->concatExpressions[] = $parser->StringPrimary();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong indentation by tab.

@stof
Copy link
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

@broncha
Copy link
Contributor Author

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

@stof
Copy link
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.

$args[] = $sqlWalker->walkStringPrimary($expression);
}

return $platform->getConcatExpression( $args );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should remove the extra spaces around $args

@stof
Copy link
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

@broncha
Copy link
Contributor Author

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.

…rguments is not known removing dependency to patch DBAL
@broncha
Copy link
Contributor Author

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

@beberlei
Copy link
Member

Merged in 4841a06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants