-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Conversation
… PredicateInterface
… PredicateInterface
…t ?) with Predicate object as-is
$select = new Select; | ||
$where = array( | ||
'name' => new Predicate\Literal("name = 'Ralph'"), | ||
'age' => new Predicate\Expression('age = ?', 33), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what does 'name' and 'age' mean here? I assume its just discarded?
@ralphschindler The key are discarded only a reference in the application to make the where, but now if I do that same search there is an unexpected answer |
Well, the options in this situation would be either to silently discard 'name' and 'age', or throw an exception. WHERE "name" = ("name" = 'Ralph') AND "age" = (age = ?) Which I agree is odd. It could be argued that Select should emit some kind of warning telling you that your keys are not necessary when using Predicates. |
This query generates error in the database, I use postgre, I will check on other database |
Right, it should, but what I am saying is that when you write this: $select = new Select;
$where = array(
'name' => new Predicate\Literal("name = 'Ralph'"),
'age' => new Predicate\Expression('age = ?', 33),
); it implies: WHERE "name" = ("name" = 'Ralph') AND "age" = (age = ?) when you read it. So shouldn't we throw an exception in this case? |
Got it, rather than accept and ignore keys To be honest I do not know, since the use of Predicate keys should ignore, ignore numerical and string, but the exception is a good idea since it generates this error in the database. What do you think? |
I personally think we should go the exception route, b/c in situations where you're not using predicates, string keys mean something specific. And if someone were to try and apply that same meaning to predicates as values, I think it would be better if we threw an exception so they could correct their usage. I'll ask around #zftalk.dev and see what people think. |
Cool, first time sending pull request, I'm trying to help on something, I'm waiting for your decision. |
Can you update it to throw an exception when a key is there? I will merge that once its in place. |
/** | ||
* @testdox unit test: Test where() will accept any array with string key (without ?) with Predicate throw Exception | ||
* @covers Zend\Db\Sql\Select::where | ||
* @expectedException Zend\Db\Sql\Exception\InvalidArgumentException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally opt for $this->setExpectedException over the annotation, but I'll make this change. Otherwise looks good, I'll merge!
I created the unit test, to show what I thought about the change.