Framework: Bindings not correctly assigned in SQL exceptions

[peterpan666]

• Laravel Version: 5.7.12 (but maybe in every versions)
• PHP Version: 7.2.1
• Database Driver & Version: MySQL 5.6.38

Description:

When an SQL exception is thrown, the bindings are not correctly assigned in the exception's message when one of the values contains a question mark ?.

Steps To Reproduce:

On a simple Laravel install, try to run the following in php artisan tinker :

App\User::create(['foo' => '?', 'email' => '[email protected]']);

You should have an Exception like this :

Illuminate/Database/QueryException with message 'SQLSTATE[42S22]: Column not found: 1054 Unknown column 'foo' in 'field list' (SQL: insert into `users` (`foo`, `email`, `updated_at`, `created_at`) values ([email protected], 2019-04-17 12:51:12, 2019-04-17 12:51:12, ?))'

Here you can see that the value of email is in foo and that created_at is empty...

[staudenmeir]

I've stumbled over this a while ago, but couldn't find a simple solution. Do you have an idea?

@driesvints This also affects the latest version.

[peterpan666]

Ok thanks for confirming it, I didn't really dug it so couldn't tell right now.

I'll try to take a look at it and maybe propose a PR...

[akiyamaSM]

I couldn't reproduce the error with laravel 5.7.28

[staudenmeir]

@akiyamaSM You can also reproduce this by calling Str::replaceArray() directly:

dd(Str::replaceArray('?', ['foo?', 'bar'], '?, ?'));

// expected: foo?, bar
// actual:   foobar, ?

[laurencei]

This "solves" it:

public static function replaceArray($search, array $replace, $subject)
{
    $placeholder = '##################$$example$$##################';

    foreach ($replace as $value) {
        $value = str_replace($search, $placeholder, $value);
        $subject = static::replaceFirst($search, $value, $subject);
    }

    return str_replace($placeholder, $search, $subject);
}

Essentially we check if the replacement text (i.e. foo?) contains our search string (i.e. ?), and if so substitute it for some really long string that would be realistically never used in real life _before_ we replace it in the main string.

Then we swap out any instances of holding text once everything else is done, giving us the intended string.

Seems to work. My main concern would be performance on a really large query or dataset.

Thoughts @staudenmeir before I submit it as a PR?

edit: and is the $placeholder value ok? It just needs to be something that is realistically never going to be the $search value or contained inside the $subject value.

[laurencei]

Results:

dd(Str::replaceArray('?', ['foo?', 'bar'], '?, ?'));
// expected: foo?, bar
// actual:   foo?, bar

and works for multiple repeats as well:

dd(Str::replaceArray('?', ['?foo???', '?bar'], '?, ?'));
// expected: ?foo???, ?bar
// actual:   ?foo???, ?bar

and

App\User::create(['foo' => '?', 'email' => '[email protected]']);

gives:

Illuminate/Database/QueryException with message 'SQLSTATE[42S22]: Column not found: 1054 Unknown column 'foo' in 'field list' (SQL: insert into `users` (`foo`, `email`, `updated_at`, `created_at`) values (?, [email protected], 2019-04-17 12:51:12, 2019-04-17 12:51:12))'

[staudenmeir]

@laurencei I don't really like this approach, I think it's unnecessarily hacky. We can fix this in a "good" way using offsets, but it's a lot of code for such a minor issue. Looks like there is no simple and elegant solution.

[laurencei]

I started doing offsets as a solution, but that actually makes the code much larger with more variables, counters etc.

Three lines of code solves it here 🤷‍♂️

Placeholders are used in other parts of the framework when doing string replacement stuff - i.e. Parent views, so it's an approach that's been used before.

[staudenmeir]

I'm not a big fan of those either ;-)

What if we use something obscure like two/three null bytes as a placeholder? That's short and shouldn't appear in real-life strings.

[laurencei]

What about ĹáŕãƲēĺ?

There's no way that appears anywhere.

The only remaining question is would the unicode chars cause an issue on any OS or the str_replace function?

Or the last option is $placeholder = 'placeholder_'.Str::random(6); and let it be unique for each function call. Not really much, if any, overhead...

[36864]

This sounds like a lot of work just to continue using replaceFirst instead of doing it properly.

public function replaceArray($search, array $replace, $subject) {
  $offset = 0;
  foreach ($replace as $value)
  {
    $position = strpos($subject, $search, $offset);
    if ($position === false) {
      break;
    }
    $subject = substr_replace($subject, $value, $position, strlen($search));
    $offset = $position + strlen($value);
  }
  return $subject;
}

>>> replaceArray('?', ['??', 'foo?', 'bar', '????'], '?, ?, ?, ?');
=> "??, foo?, bar, ????"

EDIT: realized we can break after the first failure.

Also keep in mind this is technically a bugfix, but is also a breaking change in the way replaceArray functions. Some people may be relying on the recursive replaces.

[staudenmeir]

We can also split the string and glue it back together:

$segments = explode($search, $subject);

$result = array_shift($segments);

foreach ($segments as $segment) {
    $result .= (array_shift($replace) ?? $search).$segment;
}

return $result;

[laurencei]

@36864 @staudenmeir - I tested both your solutions, and both work.

I then performance tested both, and on large datasets (with 100,000 iterations) the performance was similar.

On smaller datasets @staudenmeir was _slightly_ faster.

So if either or both of you want to submit a PR and let Taylor decide? Either way we can get this issue fixed and closed 👍