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

Change Composer hasPackage to public #53282

Merged

Conversation

buihanh2304
Copy link

@buihanh2304 buihanh2304 commented Oct 23, 2024

This method is not used in Composer class, and cannot access from outside.
So, I change it's visibility to public.

@buismaarten
Copy link
Contributor

May be a breaking change when someone is using the method in their application.

@buihanh2304
Copy link
Author

@buismaarten How can they use the method without create a new class extends Illuminate\Support\Composer, and then call to the method in that class?

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Oct 24, 2024

How can they use the method without create a new class extends Illuminate\Support\Composer, and then call to the method in that class?

That is how.

Also, if, for some reason, they are overriding that method, changing its visibility from protected to public would make their subclass crash.

I agree that is very unlikely, and would be a very niche use case. But with the new year approaching, and thus a new Laravel version approaching, I guess it is safer to send this PR to master instead.

@buihanh2304 buihanh2304 changed the base branch from 11.x to master October 24, 2024 04:04
@buihanh2304 buihanh2304 force-pushed the change-composer-hasPackage-to-public branch from a11e2bb to efbb30e Compare October 24, 2024 04:12
@buihanh2304
Copy link
Author

@rodrigopedra

I understood. I will change target branch to master.

@taylorotwell taylorotwell merged commit 0d1e2d3 into laravel:master Oct 24, 2024
31 checks passed
@buihanh2304 buihanh2304 deleted the change-composer-hasPackage-to-public branch October 24, 2024 15:24
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