Skip to content

Add *whereBoundValue functions to Where trait and interface - #165

Open
dc2xl wants to merge 2 commits into
auraphp:3.xfrom
dc2xl:SupportWhereConditionPlaceholderTriples
Open

Add *whereBoundValue functions to Where trait and interface#165
dc2xl wants to merge 2 commits into
auraphp:3.xfrom
dc2xl:SupportWhereConditionPlaceholderTriples

Conversation

@dc2xl

@dc2xl dc2xl commented May 2, 2018

Copy link
Copy Markdown

This is a working minimal implementation for #164 to get coding feedback.

Implementation is directly in the WhereTrait with a separate function name ("whereBoundValue") in order to keep the where function interfaces clean.

As alternative to the direct implementation I considered having a separate trait; so the framework user could easily decide if the functionality is wanted by composing traits. But this seems infeasible considering how the factory/implementation lookup works.

What is mainly missing is *having support.

@dc2xl

dc2xl commented May 2, 2018

Copy link
Copy Markdown
Author

Just a note what we did inhouse to extend the functionality - we extended the QueryFactory#newSelect to return a Select instance extended by a trait which contains the necessary functionality. The problem with that implementation is that extended QueryFactory hardwires the database dialect.

@harikt

harikt commented Jul 26, 2026

Copy link
Copy Markdown
Member

Hi @dc2xl, apologies this sat so long without a reply.

Picking this up while going through the open queue for 6.x. The need behind #164 is real, but I think it's since been covered by the existing where() signature — named placeholders with an array bind now expand automatically:

$select->cols(['*'])->from('t')
    ->where('c2 IN (:c2)', ['c2' => ['foo', 'bar']])
    ->where('c3 = :c3',    ['c3' => 'foo']);
SELECT
    *
FROM
    `t`
WHERE
    c2 IN (:__1__, :__2__)
    AND c3 = :c3
$select->getBindValues();
// ['__1__' => 'foo', '__2__' => 'bar', 'c3' => 'foo']

That's one call per condition with named placeholders, which was the conciseness #164 was after, and the array case is expanded into individual binds rather than left as a single array value. Scalar placeholders keep the name you wrote; array ones get rewritten to :__1__, :__2__ — so the "named for easier debugging" benefit holds for scalars but not for the expanded elements. That's the one gap versus your proposal.

Worth noting because it affects this PR directly: testWhereBoundValue() asserts getBindValues() === ['c2' => ['foo']], i.e. an array kept as a single bind value. PDO can't bind that, so the expansion above is what makes the IN case actually executable.

Given that, plus the things you already called out (no having() support yet, the XXX add type checks) and that adding two methods to WhereInterface would break anyone implementing it outside the library, I'd rather not port this to 6.x. Leaving it open for now rather than closing, in case you see a case the current where() doesn't cover — if so I'd genuinely like to hear it.

Thanks for taking the time to write it up, and again, sorry for the delay.

@harikt harikt added the Hold label Jul 26, 2026
@dc2xl

dc2xl commented Jul 27, 2026

Copy link
Copy Markdown
Author

The need behind #164 is real, but I think it's since been covered by the existing where() signature — named placeholders with an array bind now expand automatically:
...
in case you see a case the current where() doesn't cover — if so I'd genuinely like to hear it.

@harikt just note where the PR/approach offers some value: It provides name consistency at definition time (when you call the where function), so removes a minor error source. And one can easily check typing (which i referred to with the add type checks remarks) at that point with minimal parsing (basically the value has to be an array if there are parenthesis in the placeholder definition, else the value has to be primitive). In our fork, we use a more cryptic but shorter function name; we end up with shorter lines and are happy with it. I think most projects are fine without this.

@harikt

harikt commented Jul 28, 2026

Copy link
Copy Markdown
Member

cool.

May be your fork is private I believe. I will look more closely into this, what we can do regarding type check you mentioned.

Thanks again for taking your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants