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

3.x: Named placeholders only for where() et al. conditions #134

Merged
merged 11 commits into from
Mar 22, 2017

Conversation

pmjones
Copy link
Member

@pmjones pmjones commented Mar 21, 2017

In reference to #129 , this PR removes support for ?-placeholders in where(), having(), and join*() methods. You can still bind at the time of the method call, but you must specify the named placeholder to bind to. E.g., instead of this ...

$select->where('id = ?', $id_value);

... you must do this:

$select->where('id = :id', ['id' => $id_value]);

Support for sub-selects as bound values is retained.

This is obviously more typing, but is more explicit, and requires less internal calculating of placeholder names.

@harikt @taxp @pavarnos et al: let me know what you think.

@pavarnos
Copy link
Contributor

Looking good: a lot of complicated code has been deleted, which is always a reason to celebrate (can i buy you a beer?)

Is it worthwhile / useful to this to its extreme to further simplify the interface and suggest that the only place you can bind values is in bindValue() / bindValues()? It is kinda convenient being able to where() and bind all in the same function call.

https://github.com/auraphp/Aura.SqlQuery/blob/3.x-named-placeholders-only/src/AbstractQuery.php#L278 is now incorrect? Maybe grep for 'placeholder' to see if there are others?

@pmjones
Copy link
Member Author

pmjones commented Mar 22, 2017

Is it worthwhile / useful to this to its extreme to further simplify the interface and suggest that the only place you can bind values is in bindValue() / bindValues()?

That would make it more difficult to replace :placeholder with a SELECT object in the condition. However, there might be a way to do so at build() time, rather than where() et al. time. For now, I'm satisfied to leave it in place.

Maybe grep for 'placeholder' to see if there are others?

Good catch. I need to walk through docs/ again anyway, and can attend to it then.

@pmjones pmjones merged commit 98a986d into 3.x Mar 22, 2017
@harikt
Copy link
Member

harikt commented Mar 22, 2017

I am good with this.

One thing probably I missed to see is a test where the same name place holder can be replaced at multiple places.

SELECT * FROM table WHERE id = :foo AND foo = :foo 

Do we have a test that cover this already ? May be quick look in diff I probably missed.

@harikt
Copy link
Member

harikt commented Mar 22, 2017

@pmjones can we delete the branch also ?

@pmjones
Copy link
Member Author

pmjones commented Mar 22, 2017

[Can] the same name place holder can be replaced at multiple places [?]

Not in Aura.SqlQuery logic itself, no -- that has to happen at the Aura.Sql level for now. I anticipate a future change where some of the Aura.Sql "rebuilder" logic gets incorporated into Aura.SqlQuery.

@pmjones
Copy link
Member Author

pmjones commented Mar 22, 2017

can we delete the branch also ?

It's been, like, 10 minutes -- give a brother a some breathing room. ;-)

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.

3 participants