Skip to content
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

Open
wants to merge 3 commits into
base: 3.x
Choose a base branch
from

Conversation

pmjones
Copy link
Member

@pmjones pmjones commented Apr 19, 2017

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:

$select->where('foo IN(?)', [1, 2, 3]);
// WHERE foo IN (:_1_)

And then changed it to this in 3.x:

$select->where('foo IN(:foo)', ['foo' => [1, 2, 3]]);
// WHERE foo IN (:foo)

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:

$select->where('foo IN(', [1, 2, 3], ')');
// WHERE foo IN(:_1_)

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?

@pavarnos
Copy link
Contributor

Thanks for asking...
Interesting idea. Looks like your code would handle complicated stuff like the below OK

$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?

@pmjones
Copy link
Member Author

pmjones commented Apr 20, 2017

It's a BC break against both 2.x and the current 3.x, in that it uses alternating "SQL" and "value" params via ...$cond, rather than $cond, $bind = array(). The following works now in 3.x, but will not work if the PR merges:

$select->where('foo = :foo OR bar = :bar', ['foo' => 'fooval', 'bar' => 'barval']);

However, this will work with the PR:

$select->where('foo = :foo OR bar = :bar');
$select->bindValues(['foo' => 'fooval', 'bar' => 'barval']);

And so will this:

$select->where('foo = ', 'fooval', ' OR bar = ', 'barval');

You see where I'm coming from?

@pavarnos
Copy link
Contributor

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....

@pmjones
Copy link
Member Author

pmjones commented Apr 20, 2017

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.

@pavarnos
Copy link
Contributor

thanks. it will make migration from 2.x to 3.x easier i guess.
fwiw i'm living dangerously and using 3.x in production. its working well.

@djmattyg007
Copy link
Contributor

What's the purpose of tracking instance counts? Just curious as I don't think it's that obvious.

@agrrr3
Copy link

agrrr3 commented Nov 29, 2017

@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

$select->where('foo IN(', [1, 2, 3], ')');
$select->where('bla IN(:bla)');
$select->where('bar IN(', [7, 8, 9], ')');
$select->bindValue("bla", [4,5,6]);
// WHERE foo IN(:_1_) AND bar IN(:bar) AND bla IN(:_2_)
// with bind values [ "_1_" => [1,2,3], "_2_" => [7,8,9], "bla" =>[4,5,6] ]

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)

$select->where('foo IN(:bars', [1, 2, 3], ')');
// WHERE foo IN(:bars)
// bindValues: [ "bars" => [1,2,3] ]

or without back-scanning

$select->where('foo IN(', [ "bars" => [1, 2, 3] ], ')');
// WHERE foo IN(:bars)
// bindValues: [ "bars" => [1,2,3] ]

If you restrict yourself to simpler statements per where calls you end up with triples of statement-snippet, placeholder and value

$select->where("ID_KEY = ", ":ID_KEY", $val);
$select->where("fpath LIKE ", ":USERDIR", "%/user/%");
$select->where("name IN ", "(:NAMES)", ["pmjones","pavarnos"]);

@harikt
Copy link
Member

harikt commented Apr 3, 2018

Similar one #162 ?

@harikt
Copy link
Member

harikt commented Feb 4, 2022

@pmjones Any reason you didn't merge this PR?

@pmjones
Copy link
Member Author

pmjones commented Feb 4, 2022

@harikt I don't recall. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants