Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Fixed return type of Grammar::getValue #39725

Merged
merged 1 commit into from
Nov 23, 2021
Merged

[8.x] Fixed return type of Grammar::getValue #39725

merged 1 commit into from
Nov 23, 2021

Conversation

robotsniper
Copy link
Contributor

this return type should be consistent with Expression class getValue method return type

@GrahamCampbell GrahamCampbell changed the title retrun type fixed [8.x] Fixed return type of Grammar::getValue Nov 23, 2021
@taylorotwell taylorotwell merged commit 1a61d47 into laravel:8.x Nov 23, 2021
@sebdesign
Copy link
Contributor

sebdesign commented Nov 23, 2021

I would suggest that we revert this change.

The actual return type of this method depends on the return type of Expression::getValue().
While this method returns mixed, this is something that was added in the first commit of Laravel!
See: https://github.com/laravel/framework/blame/599e7851988ce88aae745847389dcefd3c4cc6f6/src/Illuminate/Database/Query/Expression.php#L26

The Expression also has a __toString() method that casts the result of getValue() to string, which is risky, because if the value cannot be casted to string, that would raise an error.

Also the result of Grammar::getValue() is used in many methods that return string, like wrapTable(), wrap(), parameter().

The most sane thing to do is to change all the references of mixed types to \Stringable|scalar in the Expression class in 9.x as it is a breaking change.

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.

4 participants