Skip to content

Fix LIMIT/OFFSET and FOR UPDATE guards firing on keywords inside identifiers, comments and subqueries. - #13

Open
itoolsTim wants to merge 1 commit into
mainfrom
bugfix/limit-offset-bug
Open

Fix LIMIT/OFFSET and FOR UPDATE guards firing on keywords inside identifiers, comments and subqueries.#13
itoolsTim wants to merge 1 commit into
mainfrom
bugfix/limit-offset-bug

Conversation

@itoolsTim

Copy link
Copy Markdown

Fix LIMIT/OFFSET and FOR UPDATE guards firing on keywords below the top level

selectOne(), queryOne() and count() append their own LIMIT, so they reject templates that already contain one. The check was preg_match('/\b(LIMIT|OFFSET)\b/i', $where), and \b matches inside backticks, comments and parentheses. So all of these threw This method doesn't support LIMIT or OFFSET on valid SQL:

DB::selectOne('pages', "`offset` = ?", 5);                              // column named offset
DB::count('pages', "status = ? /* no LIMIT here */", 'active');          // keyword in a comment
DB::queryOne("SELECT * FROM (SELECT * FROM ::log ORDER BY id DESC LIMIT ?) AS t WHERE t.level = ?", 5, 'error');
                                                                         // LIMIT belongs to the derived table

The FOR UPDATE / LOCK IN SHARE MODE check in rejectPreLimitConflicts() had the same problem.

Fix (src/ConnectionInternals.php): new private helper topLevelSql() blanks out quoted text, backtick identifiers, comments and parenthesised groups (nested, in one recursive pass). Both guards now match against that, so only a real top-level clause fires them. The raw regex still runs first, so templates with no keyword at all cost nothing extra. Error messages are unchanged. The trailing-comment and trailing-; checks deliberately still see the raw template.

Side effect: a quoted keyword like name = 'no limit' now reports the accurate Quotes not allowed in template error instead of the misleading LIMIT/OFFSET one (quotes were always rejected by assertSafeTemplate(); the guard just fired first).

Tests: tests/DB/LimitOffsetGuardTest.php (16 tests) - keyword allowed inside identifiers, comments, derived tables and nested subqueries; top-level LIMIT/OFFSET/FOR UPDATE still rejected, including after a subquery; quotes error takes precedence. Existing rejection tests in SelectTest, CountTest, QueryTest, DocsExamplesTest are unaffected.

Feel free to take whatever from this and test it out. I ran this specifically against 3.84 and based on the repo after including SmartArrayHtml and related classes.

@itoolsTim
itoolsTim requested a review from daveedis August 25, 2026 00:02
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.

2 participants