Add Expr::concat support for multiple arguments #1397

giosh94mhz · 2015-04-28T08:40:01Z

DQL CONCAT function support variable number of arguments (2+) since version 2.4.0.

The Expr class, on the other hand, is still limited to 2 arguments, and require multiple call to achive a similar result (see this SO question). I've improved Expr implementation to support variable number of arguments in a backward compatible way.

Please, consider backporting to 2.4 branch if you plan to release other versions, since 2.5 introduce many features and may not be possible to easily upgrade. I may be wrong, though.

doctrinebot · 2015-04-28T08:40:12Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3717

We use Jira to track the state of pull requests and the versions they got
included in.

MisatoTremor · 2015-09-10T11:24:04Z

lib/Doctrine/ORM/Query/Expr.php

@@ -526,13 +526,13 @@ public function notLike($x, $y)
     * Creates a CONCAT() function expression with the given arguments.
     *
     * @param mixed $x First argument to be used in CONCAT() function.
-     * @param mixed $y Second argument to be used in CONCAT() function.
+     * @param mixed $y,... Other arguments to be used in CONCAT() function.
     *
     * @return Expr\Func
     */
    public function concat($x, $y)


This should have only one argument $x like the other functions accepting multiple arguments (andX, orX and countDistinct).

Sure, I kept the minimum of 2 not to introduce BC. Not supporting 2+ arguments is a bug, since CONCAT DQL expression is now valid syntax, but supporting a single parameters is halfway a bugfix and a feature.

I'll do some test and commit the new change so that the maintainer can review.

@MisatoTremor: @giosh94mhz is correct - removing the param would be a bc break.

@zeroedin-bill well I don't know if this is strictly a BC.

I mean, to stay on the safe side the $y parameter should be kept, but at runtime is a BC only to add a parameters (w/o defaults), since PHP will ignore extra parameters by default.

You'll have a BC by overriding the method in sub classes, but since this class is part of a Builder pattern is meant to be final anyway.

Am I correct?

@giosh94mhz If the class were actually marked final, it would not matter. Given that it is not actually marked final, someone out there may have extended it. As part of the public API, the intent for this class doesn't really matter. The class is not final, so removing a parameter of this function is indeed a BC break (will raise warnings if it has been extended)

@zeroedin-bill I agree with you. I was just suggesting that the scenario is rare and not recommended due to the Builder pattern, so the maintainer may decide to merge it anyway. Also, this can be merged in master, only partially back-ported to 2.4 and/or 2.5.

giosh94mhz · 2015-09-10T13:36:51Z

I've pushed a new commit which allow CONCAT with only one argument; this is supported in DBAL, so there should be no problem. Travis will have the final answer.

Note that since this change the public method concat($x, $y) to concat($x) this MAY be considered a BC; there should be no problem at runtime, but some sub-class (in a Builder pattern?) may be affected. I'm ready to rollback and open a new PR if needed.

billschaller · 2015-09-10T13:45:00Z

lib/Doctrine/ORM/Query/AST/Functions/ConcatFunction.php

-
-        $this->secondStringPrimary = $parser->StringPrimary();
-        $this->concatExpressions[] = $this->secondStringPrimary;
+        $this->concatExpressions[] = $parser->StringPrimary();


All of the changes in this file are unnecessary - DQL CONCAT already supported >2 arguments - The semantics required at least 2 arguments. This change makes it so you can do weird things like CONCAT(field) - That isn't really valid. CONCAT should require a minimum of 2 arguments.

giosh94mhz · 2015-09-10T14:08:43Z

@zeroedin-bill Thanks for the code review, and... well I got the point. I just noted right now that you are tagged as Collaborator; I though you were just reviewing not actually preparing for merge, since @Ocramius is usually reviewing PR.

Anyway, I never found a DBMS which yell at me if I do SELECT CONCAT("a"), neither a scripting language which prevent me of doing cat file or equivalent. Don't want to start the flame, just saying that the strict requirement of 2+ arguments is only supported by the BC issue.

So, if I revert commit 6a665b8 and follow your other suggestion, this will be merged?

MisatoTremor · 2015-09-10T14:16:23Z

Sorry for having caused such a fuzz here.
I wasn't fully aware of \Expr being public, so your original changes are good in my book.

billschaller · 2015-09-10T14:30:22Z

@giosh94mhz I can guess that @Ocramius will not merge a BC break for this, as this is simply an enhancement, and probably not worth a BC break.

Also, I don't know about Oracle, but SELECT CONCAT('a') will return The concat function requires 2 to 254 arguments. on SQL Server.

giosh94mhz · 2015-09-10T15:01:52Z

SQL Server always surprise me :) I looked into the Platform and I saw that the actual concat expression is the + operator, probably to deal with concatenation of a single element.

In the end, the one-parameter scenario is too complicated: BC and Platforms issues are too much for sure. I'll rollback, the last commmit and integrate you suggestion. I'll do my best with whitespaces, but there are some CS issue (carriage-return chars, some tabs, ...).

Ocramius · 2015-11-07T04:10:57Z

Overall, looks good. @zeroedin-bill: assigning to you for final review/merge (into master - 2.6.0).

I think the test needs to be split into a separate method.

Add Expr::concat support for multiple arguments

guilhermeblanco · 2015-11-07T15:47:11Z

Thanks! =)

giosh94mhz force-pushed the concat_expr_variable_arguments branch from 07681f4 to 1a0bdae Compare April 30, 2015 08:40

giosh94mhz force-pushed the concat_expr_variable_arguments branch from 1a0bdae to 61e33e5 Compare May 15, 2015 07:43

Ocramius added the Requires merge/review label Aug 8, 2015

Ocramius self-assigned this Aug 8, 2015

MisatoTremor reviewed Sep 10, 2015
View reviewed changes

giosh94mhz force-pushed the concat_expr_variable_arguments branch from 61e33e5 to 6a665b8 Compare September 10, 2015 13:31

billschaller reviewed Sep 10, 2015
View reviewed changes

billschaller added the Delayed label Sep 10, 2015

Add Expr::concat support for multiple arguments

1617253

giosh94mhz force-pushed the concat_expr_variable_arguments branch from 6a665b8 to 1617253 Compare September 10, 2015 15:06

Ocramius assigned billschaller and unassigned Ocramius Nov 7, 2015

Ocramius removed the Delayed label Nov 7, 2015

guilhermeblanco added a commit that referenced this pull request Nov 7, 2015

Merge pull request #1397 from giosh94mhz/concat_expr_variable_arguments

d7a3154

Add Expr::concat support for multiple arguments

guilhermeblanco merged commit d7a3154 into doctrine:master Nov 7, 2015

Ocramius unassigned billschaller Nov 7, 2015

Ocramius assigned guilhermeblanco Nov 7, 2015

doctrinebot mentioned this pull request Dec 6, 2015

DDC-3717: [GH-1397] Add Expr::concat support for multiple arguments #4553

Closed

giosh94mhz deleted the concat_expr_variable_arguments branch December 15, 2015 10:38

lcobucci added Improvement and removed Requires merge/review labels Dec 19, 2017

lcobucci added this to the 2.6.0 milestone Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Expr::concat support for multiple arguments #1397

Add Expr::concat support for multiple arguments #1397

giosh94mhz commented Apr 28, 2015

doctrinebot commented Apr 28, 2015

MisatoTremor Sep 10, 2015

giosh94mhz Sep 10, 2015

billschaller Sep 10, 2015

giosh94mhz Sep 10, 2015

billschaller Sep 10, 2015

giosh94mhz Sep 10, 2015

giosh94mhz commented Sep 10, 2015

billschaller Sep 10, 2015

giosh94mhz commented Sep 10, 2015

MisatoTremor commented Sep 10, 2015

billschaller commented Sep 10, 2015

giosh94mhz commented Sep 10, 2015

Ocramius commented Nov 7, 2015

guilhermeblanco commented Nov 7, 2015

Add Expr::concat support for multiple arguments #1397

Add Expr::concat support for multiple arguments #1397

Conversation

giosh94mhz commented Apr 28, 2015

doctrinebot commented Apr 28, 2015

MisatoTremor Sep 10, 2015

Choose a reason for hiding this comment

giosh94mhz Sep 10, 2015

Choose a reason for hiding this comment

billschaller Sep 10, 2015

Choose a reason for hiding this comment

giosh94mhz Sep 10, 2015

Choose a reason for hiding this comment

billschaller Sep 10, 2015

Choose a reason for hiding this comment

giosh94mhz Sep 10, 2015

Choose a reason for hiding this comment

giosh94mhz commented Sep 10, 2015

billschaller Sep 10, 2015

Choose a reason for hiding this comment

giosh94mhz commented Sep 10, 2015

MisatoTremor commented Sep 10, 2015

billschaller commented Sep 10, 2015

giosh94mhz commented Sep 10, 2015

Ocramius commented Nov 7, 2015

guilhermeblanco commented Nov 7, 2015