Skip to content

Conversation

@ivanrussu
Copy link

Hello! I noticed that if an Eloquent query contains comments, the EloquentDataSource::createRunnableQuery function handles bindings replacement incorrectly.

Case 1

When using named bindings and having a question mark inside SQL comments, an exception occurs:
Undefined array key 0 at EloquentDataSource.php:274,
because the function tries to replace the ? with a positional binding, but there is no binding with key 0.

How to reproduce:

$bindings = [
    'id' => 1
];

$query = <<<SQL
    SELECT
        *
    FROM
        users -- is that correct table?
    WHERE
        id = :id
SQL;

DB::select($query, $bindings);

Case 2

When using positional bindings and having a question mark inside SQL comments, the function incorrectly replaces question marks in the comment with bindings, instead of replacing only the real ? in the query.

How to reproduce:

$bindings = [
    1
];

$query = <<<SQL
    SELECT
        *
    FROM
        users -- is that correct table?
    WHERE
        id = ?
SQL;

DB::select($query, $bindings);

Result:

SELECT
    *
FROM
    users -- IS that correct table1
WHERE
    id = ?

Fix

I propose to determine whether the bindings are positional before performing the replacements.

Eloquent allows queries to use either positional or named parameters.
I check whether the bindings are positional by verifying that the bindings array contains only numerically ordered keys.

The updated regular expression now ignores placeholders inside:

  • Single-line comments (-- ...)
  • Multi-line comments (/* ... */)
  • Single-quoted string literals ('...')
  • Double-quoted string literals ("...")

This ensures that placeholders are replaced only where intended, preventing accidental replacements inside comments or strings.

Previously, `createRunnableQuery()` incorrectly replaced placeholders (`?` or `:name`)
found inside SQL comments or string literals. This caused:

- For named bindings: "Undefined array key 0" when a `?` appeared in a comment.
- For positional bindings: placeholders in comments being replaced with bindings.

Changes:
- Detect positional vs named bindings before replacement.
- Updated regex to skip:
  * Single-line comments (`-- ...`)
  * Multi-line comments (`/* ... */`)
  * Single-quoted strings (`'...'`)
  * Double-quoted strings (`"..."`)
- Ensures placeholders inside these constructs are ignored.

This prevents incorrect binding substitution and avoids exceptions when
queries contain comments or strings with question marks.
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.

1 participant