feat: Allow RawSql JOIN tables (#9070) - #10571
wakqasahmed wants to merge 2 commits into
Conversation
wakqasahmed
left a comment
There was a problem hiding this comment.
Independent review found one reproducible alias-tracking issue with DBPrefix (inline below). Verified the affected JOIN compilation on PHP 8.2 in a disposable container. The Postgre and SQLSRV signature changes and raw-target bypass are consistent with the new API, and the 4.8 upgrade notes cover subclass compatibility. Remaining validation gap: no live PostgreSQL or SQL Server execution performed during this review.
| protected function trackAliases($table) | ||
| { | ||
| if ($table instanceof RawSql) { | ||
| if (preg_match('/\)\s+(?:AS\s+)?([a-z_][a-z0-9_]*)\s*$/i', (string) $table, $matches)) { |
There was a problem hiding this comment.
[P2] Track aliases for other valid raw JOIN targets
The new matcher only recognizes an unquoted alias immediately following ). With DBPrefix = "ci_", join(new RawSql("posts recent"), "recent.user_id = users.id") compiles to JOIN posts recent ON "ci_recent"."user_id" = "ci_users"."id", referring to an alias that does not exist. The same failure occurs for a protected subquery alias such as new RawSql('(SELECT user_id FROM posts) AS "recent"'). Both were reproduced on PHP 8.2; the existing unquoted subquery case passes. Please recognize terminal aliases for ordinary raw table expressions and quoted subquery aliases, and add prefix regressions for these cases so the string condition retains the actual alias.
There was a problem hiding this comment.
Fixed in 9d63234. Raw JOIN alias tracking now recognizes ordinary table aliases and quoted subquery aliases, with DBPrefix regressions for both cases. JoinTest passes (16 tests); PHP 8.2 syntax and file-scoped style checks pass.
|
The updated JOIN regressions pass locally (16 tests), and the review fix is in 9d63234. The Oracle CI job failed in the randomized AutoReview test |
Description
RawSqlas the JOIN table, including in the Postgre and SQLSRV builders.The new base test failed with the original
TypeErrorbefore the change. The local PHP image has no coverage driver, so coverage is left to CI.Closes #9070.
Test plan:
JoinTest.php(14 tests)Checklist: