feat: add closure-based Query Builder join conditions#10186
Conversation
paulbalandan
left a comment
There was a problem hiding this comment.
Few comments below. Also, you noted you updated Model but I don't see any changes.
- Add JoinClause for protected JOIN ON column and value conditions - Support grouped and nested join conditions with Query Builder-style methods - Share join condition compilation with SQLSRV - Document the closure join API and add focused builder coverage Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
5d80b80 to
c0bf639
Compare
Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
Thanks for catching that. I removed the Model PHPDoc change to keep this PR focused on Query Builder behavior and avoid unrelated Psalm noise, so there are no Model changes now. I also updated the PR body to remove that mention. |
Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
michalsn
left a comment
There was a problem hiding this comment.
I'm gonna be honest. I'm not a big fan of this PR.
The closure syntax is nice, but JoinClause effectively creates a second condition-building API next to Query Builder. Once we add JoinClause::where(), users will reasonably expect it to behave like normal $builder->where() and to add related methods like like(), whereIn(), etc.
Also, we already have inconsistent behavir:
$builder->where('job.status LIKE', 'p%'); // works
$join->where('job.status LIKE', 'p%'); // compiles incorrectly
$builder->where('job.deleted_at =', null); // works
$join->where('job.deleted_at =', null); // compiles incorrectlyI don't think we should duplicate Query Builder condition parsing inside JoinClause. The only version I would be comfortable with is one where JOIN conditions go through the same condition-building logic the Query Builder already uses, so behavior stays consistent across the framework.
But that would require a large rewrite, which likely cannot be done without a BC break.
|
Thanks for being direct. I agree with this concern. I also agree that duplicating Query Builder condition parsing inside I'd like to first investigate whether condition building/compilation can be made reusable internally without changing existing behavior. If that foundation can be done cleanly, then this PR can come back on top of it, with JOIN conditions using the same logic as the rest of Query Builder. I'd appreciate any prior tips, guidance, or suggestions from anyone with deeper experience in the framework codebase before I start looking into it. |
Description
This PR proposes a closure-based way to build Query Builder
JOIN ONclauses.Today, simple joins are easy:
But once a join needs bound values,
ORconditions, or groupedON (...)logic, users often have to fall back to longer raw SQL strings orRawSql. That works, but it gives up some of the safety and readability the Query Builder usually provides.This PR keeps the existing
join()API intact and adds a native option for those more complex join conditions:For grouped conditions:
This produces a protected, bound
JOIN ONclause without requiring users to manually write the whole condition as raw SQL.The closure receives a
JoinClauseobject with familiar Query Builder-style methods:on()/orOn()for column comparisonswhere()/orWhere()for bound value comparisonsgroupStart()/orGroupStart()/notGroupStart()/orNotGroupStart()/groupEnd()for grouped conditionsExisting string and
RawSqljoin conditions continue to work as before.RawSqlremains the explicit escape hatch for cases that do not fit this small join-condition API.Why
This helps real-world applications express safer and clearer joins, especially when joins include conditional matching, status filters, soft-delete checks, tenant constraints, or grouped
ORlogic inside theONclause.Instead of choosing between a simple protected join string and a fully raw condition, users get a CodeIgniter-native middle path that keeps identifiers protected and values bound.
Changes
CodeIgniter\Database\JoinClause.BaseBuilder::join().Checklist: