-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Go back to pseudo-sequential where() and having() placeholders? #142
base: 3.x
Are you sure you want to change the base?
Conversation
Thanks for asking... $select->join('left', 'person_travel', 'person_travel.person_id = person.id and person_travel.finishes_on >= :startDate and person_travel.finishes_on <= :endDate') or $select->where('(finish_date >= :finishDate or finish_date = 0)'); (note the brackets to create the OR condition: all the other where() clauses are ANDed). I kinda like the verbose: it makes it clear what goes where. It also looks like your code is not a BC break (?) so it won't matter too much if people don't want to use it that way? |
It's a BC break against both 2.x and the current 3.x, in that it uses alternating "SQL" and "value" params via
However, this will work with the PR:
And so will this:
You see where I'm coming from? |
yep. thanks for the extra explanation. on further thought i quite like the new syntax: it is clean, lightweight, flows well, and makes good use of new language features. Can some assertions be added to help manage the BC breaks? eg if count($cond) == 2 && is_string($cond[0]) && is_array($cond[1]) then this probably won't work like you expect.... |
Let me see about 2.x breaks; 3.x has not had a release, so technically it's not a BC break -- but still, there might be a reasonable way to make an allowance. |
thanks. it will make migration from 2.x to 3.x easier i guess. |
What's the purpose of tracking instance counts? Just curious as I don't think it's that obvious. |
@djmattyg007 i think tracking instance counts makes it in the end easier to find out what values where bound to which parameter because they are named
What I would like even more is if you could optionally do the naming using back-scanning (the placeholder must be at the end of the partial string)
or without back-scanning
If you restrict yourself to simpler statements per where calls you end up with triples of statement-snippet, placeholder and value
|
Similar one #162 ? |
@pmjones Any reason you didn't merge this PR? |
@harikt I don't recall. :-/ |
Hi all, esp. @pavarnos --
Earlier, we removed ?-placeholder replacements from where(), having(), etc., and replaced it with binding of named parameters as part of the method call. So, in 2.x we had this:
And then changed it to this in 3.x:
After having used this for a while, I'm unsatisfied with it. It feels clunky and verbose.
This PR kind-of goes back to pseudo-sequential placeholders. However, instead of ?-placeholders, the conditions are alternately interpreted as literals and bind-values:
This feels a little bit better to me overall. It also makes it so that the SqlQuery code does not need to scan through for ?-marks, which means the various Postgres JSON query syntaxes are left untouched.
Any thoughts here?