Closed pmjones closed 7 years ago
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?
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.
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.
@pmjones can we delete the branch also ?
[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.
can we delete the branch also ?
It's been, like, 10 minutes -- give a brother a some breathing room. ;-)
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 ...
... you must do this:
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.