-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Zend\Db\Predicates - allow use different types of arguments in any position #5320
Conversation
$res .= $this->resolveSqlString($query) . ';'; | ||
} | ||
return $res; | ||
} elseif($sql instanceof SqlInterface) { |
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.
space after elseif and before open parenthesis
I stopped looking up the code because it's breaking BC too much, you can't alter method declarations like this (method names, arguments) or even strip interfaces out of nowhere, they're here for a reason :) |
simplification and cs fixes |
I'm not quite sure what the whole of the use cases are, but I see a few things wrong with the code in here. Namely, it looks like Zend\Db\Adapter has an awareness of Zend\Db\Sql components. Zend\Db\Sql components should have a dependency on Zend\Db\Adapter, but not visa-versa. Can you better explain the use cases with some examples and perhaps we can find a better way to implement them? |
This PR is child of the #5306. |
@ralphschindler simplified |
@ralphschindler Can you update with the appropriate milestone please, and/or close? |
self::TYPE_VALUE, | ||
); | ||
|
||
protected function normalizeArgument($argument, $defaultType = self::TYPE_VALUE) |
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.
Add a docblock, please.
@turrsis From what I can see, this is mainly a refactoring in order to reduce duplication; I don't see any new functionality. Is that correct? If so, I can schedule for 2.4.0. |
Yes, this is refactoring and reduce duplication. |
…chains that should never be merged
…-position' into develop Close #5320
Manually merged in 35ec442 Thanks! |
for more readable methods calls
implements Case predicate to demonstrate the need for these changes