Allow use array and multi arguments #100
Conversation
|
@vlastv please provide a test case as well. BTW why do you need that? Can't you use |
|
Variable-length arguments implemented in PHP 5.6, library can work with PHP 5.5 based on composer.json. I use in case: $restrictions = [];
foreach($filters as $field => $value) {
$restrictions[] = Criteria::expr()->eq($field, $value);
}
$criteria->andWhere(call_user_func_array([Criteria::expr(), 'andX'], $restrictions]));IMHO, it variant is more readable: // loop
$criteria->andWhere(Criteria::expr()->andX($restrictions))); |
|
@vlastv can be bumped to PHP 5.6 as minimum requirement |
|
@Ocramius If you change the minimum version then this request can be rejected. |
|
@vlastv I'm suggesting to add it to this patch ;-) |
| @@ -42,7 +42,7 @@ class ExpressionBuilder | |||
| */ | |||
| public function andX($x = null) | |||
| { | |||
| return new CompositeExpression(CompositeExpression::TYPE_AND, func_get_args()); | |||
| return new CompositeExpression(CompositeExpression::TYPE_AND, is_array($x) ? $x : func_get_args()); | |||
There was a problem hiding this comment.
This should be changed to just accept $x, and the method signature should become ...$x
| @@ -52,7 +52,7 @@ public function andX($x = null) | |||
| */ | |||
| public function orX($x = null) | |||
| { | |||
| return new CompositeExpression(CompositeExpression::TYPE_OR, func_get_args()); | |||
| return new CompositeExpression(CompositeExpression::TYPE_OR, is_array($x) ? $x : func_get_args()); | |||
There was a problem hiding this comment.
This should be changed to just accept $x, and the method signature should become ...$x
| ); | ||
| $this->assertEquals($x, $expr->getExpressionList()); | ||
|
|
||
| $expr = $this->builder->andX($x); |
| ); | ||
| $this->assertEquals($x, $expr->getExpressionList()); | ||
|
|
||
| $expr = $this->builder->orX($x); |
| @@ -40,19 +40,19 @@ class ExpressionBuilder | |||
| * | |||
| * @return CompositeExpression | |||
| */ | |||
| public function andX($x = null) | |||
| public function andX(...$x) | |||
There was a problem hiding this comment.
@Ocramius should we use Expression ...$x as well (and drop the mixed)?
There was a problem hiding this comment.
I'm fine with Expression ...$x, but it will break on HHVM ;-)
There was a problem hiding this comment.
bad idea: we allow strings too, not only Expression objects (as Expression objects are just here to be casted in the end anyway). Typehinting as Expression would be a huge BC break.
There was a problem hiding this comment.
@stof are you sure about strings?
seems to say something different...There was a problem hiding this comment.
sorry, I confused the Criteria system in doctrine/collection with the expression builder of the QueryBuilder. Forget my comment.
|
The current state of the PR does not change the API at all. It still does not allow to pass a single array instead of variadic arguments. So it has no benefit for people using Doctrine (it does not allow to do what the description says). @vlastv if your own code runs on PHP 5.6+, you can avoid // loop
$criteria->andWhere(Criteria::expr()->andX(...$restrictions))); |
|
So the only thing achieved by your PR so far is breaking support for HHVM if you typehint, without any benefit for users of the API... |
|
I close this PR? |
|
Now that we dropped HHVM support we can rebase this branch and be more strict on the typehint... |
|
This can only target 2.x, sadly. See https://3v4l.org/QIi9Y - it is a BC break due to the |
No description provided.