Skip to content

Conversation

@mfaas89
Copy link

@mfaas89 mfaas89 commented Feb 26, 2025

Discussion: #54814

Issue:

The exception I got was: "SQLSTATE[H093] Invalid parameter number (Connection: mysq, select ...)". There where no reserved keywords.

The query builder throws errors in some cases where backticks are not used on raw queries on execution.
If you execute the dumped query by sentry it will execute in mysql.

Products:

  • Mysql 8
  • php8.3
  • laravel 10

Solution:

Enforce backticks on raw expressions.
Remove all backticks and add them where the combination of table and column are used.

$value = 'ROUND(cs`.credit)';

var_dump( preg_replace(
	'/(\w+)[.](\w+)+/',
	'`$1`.`$2`',
	str_replace('`', '', $value)
));
public function wrap($value, $prefixAlias = false)
    {
        if ($this->isExpression($value)) {
            return $this->wrapRaw(
                (string) $this->getValue($value)
            );
        }

        .....
    }

public function wrapRaw(string $value): string
    {
        return preg_replace(
            '/(\w+)[.](\w+)+/',
            '`$1`.`$2`',
            str_replace('`', '', $this->getValue($value))
        );
    }

@rodrigopedra
Copy link
Contributor

$value = 'ROUND(cs`.credit)';

What is this supposed to do?

It is clearly an incorrect expression. There is an unmatched backtick.

Why should the query builder try to autocorrect it, instead of error out?

@rodrigopedra
Copy link
Contributor

Task: create an SQL expression to replace all backticks on a column by a single quote.

<?php

$input = "REPLACE(table.column, '`', '''')";

$output = preg_replace(
    '/(\w+)[.](\w+)+/',
    '`$1`.`$2`',
    str_replace('`', '', $input)
);

var_dump($output);

assert(
    $output === "REPLACE(`table`.`column`, '`', '''')",
    'Ooops! Too much was replaced!'
);

Output:

$ php test.php 
string(35) "REPLACE(`table`.`column`, '', '''')"
PHP Fatal error:  Uncaught AssertionError: Ooops! Too much was replaced! 

@mfaas89
Copy link
Author

mfaas89 commented Feb 27, 2025

$value = 'ROUND(cs`.credit)';

What is this supposed to do?

It is clearly an incorrect expression. There is an unmatched backtick.

Why should the query builder try to autocorrect it, instead of error out?

The exception I got was: "SQLSTATE[H093] Invalid parameter number (Connection: mysq, select ...)". There where no reserved keywords.

It did not have any reserved words inside it. When I used the query from sentry it still executed in mysql, but it did not when trying to use the eloquent get() method.
When I changed the tables to use backticks it worked.

I figured this would be an easy way to make sure all table.columns get changed into table.column. The str_replace was more of a lazy feature, which you are right should just error out.

@mfaas89
Copy link
Author

mfaas89 commented Feb 27, 2025

I also wanted to know, if people agreed with my approach before fixing tests and styling

@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

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.

3 participants