Support for comparing multivalue numeric fields #243
Conversation
| if ($this->platform instanceof MySqlPlatform) { | ||
| switch ($operator) { | ||
| case '=': | ||
| case '!=': |
There was a problem hiding this comment.
Ignore this switch case..
lsmith77
changed the title
|
if it fixes the issue, then its fine to me. however I wonder if we have the issue with SQLite/PostgreSQL as well? I guess a test case makes sense .. |
|
I imagine we will, lets add a test case and see how it goes. |
|
seems to only work for postgres. and we seem to have some issue with reloading fixtures. |
|
Thats odd, I am pretty sure I developed it in MySQL. Will have another look soon regardless. |
|
i added this to the 1.2 milestone, as lukas says this is important for sulu. is it? |
|
yeah, it would be very useful /cc @wachterjohannes |
dantleech
force-pushed
the
compare_multivalue_integer
branch
from
6444738 to
520d698
Compare
|
ok this should pass now, although I suspect we still have a flaky test lurking in master. ( This PR also makes me think that we should handle the query building with events, or at least provide a more structured way of specializing queries for different drivers. Maybe something to think about. |
|
the flaky test is noted in #239 |
|
mysql reliably fails. is this related to what you are doing or not? |
|
yeah sorry -- just found out I was only testing locally on sqlite. debugging MySQL now. |
|
ok, builds now passing apart from the flaky sqlite. |
| $operator . " " . | ||
| $literalOperand->getLiteralValue(); | ||
|
|
||
| if ($this->platform instanceof MySqlPlatform) { |
There was a problem hiding this comment.
to make this more readable, could we do?
if ($this->platform instanceof MySqlPlatform && in_array($operator, array('=', '!='))) {
return sprintf(...
}
|
great. apart from the readability i think this is good now. i really would merge the if and the switch into one, about the assignment vs direct return i'd prefer the direct return but don't mind too much. can you then squash the commits please? |
dantleech
force-pushed
the
compare_multivalue_integer
branch
from
eeaf124 to
91072a2
Compare
|
Restructured the code. Also found out that |
dantleech
force-pushed
the
compare_multivalue_integer
branch
from
91072a2 to
2f2a7a0
Compare
|
and squashed. |
| $operator . " " . | ||
| $literalOperand->getLiteralValue(); | ||
|
|
||
| if ($this->platform instanceof MySqlPlatform && $operator == '=') { |
There was a problem hiding this comment.
if ($this->platform instanceof MySqlPlatform && $operator == '=') {
=>
if ($this->platform instanceof MySqlPlatform && '=' === $operator) {
There was a problem hiding this comment.
Does === offer any advantage here? Changed anyway.
There was a problem hiding this comment.
its slightly faster and of course its type safe. I just find it a good practice to "default" to ===
In JCR-SQL2 queries
dantleech
force-pushed
the
compare_multivalue_integer
branch
from
2f2a7a0 to
594b8cd
Compare
|
good to merge? |
[POC] Support for comparing multivalue numeric fields
|
great, thanks a lot! |
dbu
changed the title
This is just a POC for now.