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

addSelect behaviour of Builder fixed #8048

Closed
wants to merge 2 commits into from

Conversation

DusanKasan
Copy link

Fix for #6513

@taylorotwell
Copy link
Member

There is no bug to fix. They just need to use a select statement.

@zealouscreations
Copy link

There is no bug to fix. They just need to use a select statement. -- @taylorotwell

I disagree with this. It may not be a 'bug' per se, but it is an incorrect use of nomenclature, and thus a fix should have been considered. You, essentially, believe that users of your framework should include the 'select all' * in the addSelect() to make sure the assumed behavior of the Eloquent query stays in tact. But this goes against the natural meaning of the wording of that method. When I use User::get() I know that this will 'get' all of the users with all of the columns in users table - without having to "use a select statement". So the natural language of User::addSelect([DB::raw("'foo' as my_custom_column"])->get() should 'add' a custom column select to the assumed selection of all columns, because I didn't already override the default select logic with the select() method.

I believe it was a mistake to ignore this observation from your users made over 7 years ago. Because making this change now could possible be a breaking change, or at the very least expose columns that people may not have wanted retrieved from the database. But I stand by my assessment of this, and by all the others that have made this observation that you shrugged off with "just use a select statement". @DusanKasan's commit here was a good approach to addressing this, and I wish you would have giving it more serious consideration.

Here I am after 5+ years of using Laravel, and I have to remember that using addSelect() will wipe out the select all default in the query builder. Then having to google why this is the case, and find this again - as I believe have done a couple times before. Where if the language in this method was used as it sounds (as Laravel tends to do very well), I would have just kept on coding without skipping a beat.

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