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

[11.x] convert collect() helper to new Collection() #53726

Merged
merged 15 commits into from
Dec 3, 2024

Conversation

browner12
Copy link
Contributor

continuation of #53563

Copy link

github-actions bot commented Dec 2, 2024

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

i screwed up when I did the merge
@chu121su12
Copy link
Contributor

How about converting also the Collection::make calls?

public static function make($items = [])
{
return new static($items);
}

@tontonsb
Copy link
Contributor

tontonsb commented Dec 2, 2024

If this is actually needed, this should be done using a custom PHP CS Fixer rule as that would also enforce it going forward. I certainly would just do collect() if I were to contribute new code that needs collecting.

Personally I think framework itself should take advantage of all the helpers as a proving ground, unless there's a serious performance hit in something like loops.

@browner12 browner12 marked this pull request as ready for review December 2, 2024 20:30
@browner12
Copy link
Contributor Author

@chu121su12 I can try that in a separate PR.

@tontonsb I would love to add automation for this. Any thoughts on how to get started?

@tontonsb
Copy link
Contributor

tontonsb commented Dec 2, 2024

@browner12 sorry, it looks harder than I assumed. The laravel/framework repo is styled by StyleCI and I don't think it allows adding custom fixers. Maybe @GrahamCampbell can tell if such a fixer can be included.

browner12 added a commit to browner12/framework that referenced this pull request Dec 2, 2024
while there is no behavior change in this commit, my intent behind this is to help prevent stupid mistakes from assuming what Collection is being referenced. by always aliasing this class explicitly, we gain a little confidence when in the code about what version we are referencing.

our use of `Illuminate\Support\Collection` far outweights our use of `Illuminate\Database\Eloquent\Collection`. however, it's very common for both to be used in a file that uses `Illuminate\Database\Eloquent\Collection`. therefore, I say we treat the base Collection as the default, and our Eloquent Collection as our alias ALL the time. this will provide consistency and help avoid stupid mistakes.

it is also helpful when doing global searches because now we have a unique token to search for.

this idea came to me as I was working on laravel#53726 because making sure I was referencing the correct Collection was the gotcha I had to pay the closest attention to.
taylorotwell pushed a commit that referenced this pull request Dec 3, 2024
while there is no behavior change in this commit, my intent behind this is to help prevent stupid mistakes from assuming what Collection is being referenced. by always aliasing this class explicitly, we gain a little confidence when in the code about what version we are referencing.

our use of `Illuminate\Support\Collection` far outweights our use of `Illuminate\Database\Eloquent\Collection`. however, it's very common for both to be used in a file that uses `Illuminate\Database\Eloquent\Collection`. therefore, I say we treat the base Collection as the default, and our Eloquent Collection as our alias ALL the time. this will provide consistency and help avoid stupid mistakes.

it is also helpful when doing global searches because now we have a unique token to search for.

this idea came to me as I was working on #53726 because making sure I was referencing the correct Collection was the gotcha I had to pay the closest attention to.
@taylorotwell taylorotwell merged commit 7db7ea9 into laravel:11.x Dec 3, 2024
38 checks passed
@browner12 browner12 deleted the AB-more-collect branch December 3, 2024 16:26
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