Replaced func_get_args() with variadic arguments #3831
Conversation
morozov
force-pushed
the
issues/3825
branch
2 times, most recently
from
827eb12 to
999065a
Compare
| $selects = is_array($select) ? $select : func_get_args(); | ||
|
|
||
| return $this->add('select', $selects, true); | ||
| return $this->add('select', array_merge([$expression], $expressions), true); |
There was a problem hiding this comment.
This is changing the behaviour since the first argument could be an array already.
There was a problem hiding this comment.
I should have probably changed the parameter type in the annotation from mixed to string. This is already reflected in the upgrade notes and there's another PR coming: morozov/dbal@issues/3825...morozov:issues/3825-NEXT.
There was a problem hiding this comment.
I enforced (actually, PHP_CodeSniffer forced me to) all expressions to be strings.
| $groupBy = is_array($groupBy) ? $groupBy : func_get_args(); | ||
|
|
||
| return $this->add('groupBy', $groupBy, false); | ||
| return $this->add('groupBy', array_merge([$expression], $expressions), false); |
| $groupBy = is_array($groupBy) ? $groupBy : func_get_args(); | ||
|
|
||
| return $this->add('groupBy', $groupBy, true); | ||
| return $this->add('groupBy', array_merge([$expression], $expressions), true); |
morozov
force-pushed
the
issues/3825
branch
2 times, most recently
from
3f16961 to
8744c78
Compare
| @@ -501,21 +499,16 @@ public function distinct() : self | |||
| * ->leftJoin('u', 'phonenumbers', 'u.id = p.user_id'); | |||
| * </code> | |||
| * | |||
| * @param mixed $select The selection expression. | |||
| * @param string $expression The selection expressions. | |||
There was a problem hiding this comment.
| * @param string $expression The selection expressions. | |
| * @param string $expression The selection expression. |
There was a problem hiding this comment.
I think this makes sense, because the array support really only exists because at the time we added this feature the ... syntax didn't exist yet.
But it will be essential that we trigger_error(E_USER_DEPRECATED) here, because this only deprecates half the method, and its not easy to find these in your own code base given how this is probably spread in hundreds of places for a mid-sizied application that uses DBAL QueryBuilder.
This was my thought too. The current API allows mixing arrays and variadic arguments and will silently drop the latter in favor of the former. |
What I'm not really sure is how seeing a deprecation error while upgrading to a minor version is better from the developer experience standpoint than seeing failure after an upgrade to the next major. In both cases, the code in question will need to be triggered either by a test or manually. Upgrading to a new major w/o testing the code is irresponsible, and runtime errors won't save you from that. Besides that, in order to see an error suppressed with |
Fixes #3825
In addition to the method signature changes, the following changes have been made:
where(),having()and theirand*andor*counterparts have been refactored to reduce code duplication.QueryBuildertests that call$qb->expr()but don't use the return value have been cleaned up.