Skip to content

[13.x] fix(postgres): whereDate and whereTime crash when $column is an Expression#60305

Open
ahawlitschek wants to merge 1 commit into
laravel:13.xfrom
ahawlitschek:fix/where-date-expression-argument
Open

[13.x] fix(postgres): whereDate and whereTime crash when $column is an Expression#60305
ahawlitschek wants to merge 1 commit into
laravel:13.xfrom
ahawlitschek:fix/where-date-expression-argument

Conversation

@ahawlitschek
Copy link
Copy Markdown

The ->whereDate and ->whereTime functions are typed to accept Expression or string as first parameter ($column). Since Laravel 12, both underlying functions in PostgresGrammar checks whether the $column is a JSON selector using the isJsonSelector($value) function defined in the base grammar. This function uses str_contains and causes a runtime exception if it receives something other than string. Currently, the function is called with the origin value of the $column parameter which in some cases may be Expression.

This adds an is_string check to only call isJsonSelector if the type of the column is a string and also adds tests which check, that using Expression in whereDate and whereTime produces proper SQL.

I discussed several ways to fix this with my colleagues. Swapping $this->isJsonSelector($where['column']) with $this->isJsonSelector($column) would also fix it. But in my opinion an Expression is semantically no json selector and the developer is responsible for ensuring a valid query if DB::raw() is used.
Changing isJsonSelector to also handle Expression seems too broad.
Another approach would be to always add parentheses within the return statement, since the parentheses are necessary because of the cast. But then more tests need to be adjusted and the created SQL will contain unnecessary parentheses most of the time.

The `->whereDate` and `->whereTime` functions are typed to accept `Expression` or string as first parameter (`$column`). Since Laravel 12, both underlying functions in `PostgresGrammar` checks whether the `$column` is a JSON selector using the `isJsonSelector($value)` function defined in the base grammar. This function uses `str_contains` and causes a runtime exception if it receives something other than string. Currently, the function is called with the origin value of the `$column` parameter which in some cases may be `Expression`.

 This adds an `is_string` check to only call `isJsonSelector` if the type of the column is a string and also adds tests which check, that using `Expression` in `whereDate` and `whereTime` produces proper SQL.

 I discussed several ways to fix this with my colleagues. Swapping `$this->isJsonSelector($where['column'])` with `$this->isJsonSelector($column)` would also fix it. But in my opinion an `Expression` is semantically no json selector and the developer is responsible for ensuring a valid query if `DB::raw()` is used.
 Changing `isJsonSelector` to also handle `Expression` seems too broad.
 Another approach would be to always add parentheses within the return statement, since the parentheses are necessary because of the cast. But then more tests need to be adjusted and the created SQL will contain unnecessary parentheses most of the time.
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