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

[6.x] Remove Container dependency on Illuminate\Support #30518

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@iansltx
Copy link
Contributor

iansltx commented Nov 5, 2019

For standalone projects, pulling in all of Illuminate\Support is a bit overkill, given that we're using only a couple of functions from the package. This chage opts to duplicate the two helper functions inside the container namespace (so they'll be picked up by a subtree split), which also removes reliance on any custom global functions.

This goes beyond effectively reverting #29959, as the container used Arr::wrap() before adding in the value() helper.

Tests are carbon copies of the equivalent Illuminate\Support tests.

@iansltx iansltx force-pushed the iansltx:remove-container-support-dep branch from 4ee47d7 to 2af9ca9 Nov 5, 2019
For standalone projects, pulling in all of Illuminate\Support is a bit
overkill, given that we're using only a couple of functions from the
package. This commit opts to duplicate the two helper functions inside
the container namespace (so they'll be picked up by a subtree split),
which also removes reliance on any custom global functions.

This goes beyond effectively reverting #29959, as the container used
Arr::wrap() before adding in the value() helper.
@iansltx iansltx force-pushed the iansltx:remove-container-support-dep branch from 2af9ca9 to 534990b Nov 5, 2019
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Nov 5, 2019

Maybe it's worth Laravel making a lighter support component, with fewer dependencies, and then have the main support component depend on that? It seems odd for the framework code to copy bits of itself into components, instead of using dependencies properly?

@GrahamCampbell GrahamCampbell changed the title Remove Container dependency on Illuminate\Support [6.x] Remove Container dependency on Illuminate\Support Nov 5, 2019
@iansltx

This comment has been minimized.

Copy link
Contributor Author

iansltx commented Nov 5, 2019

My solution is definitely a half measure, but I wasn't about to propose a significant architectural change to how illuminate/support is built, as it isn't really my place. Seems like the half measure would be reasonable for now, and when e.g. arrays and the value() helper (which should be namespaced) get split out those support components could be pulled in at that point in lieu of the custom methods.

@antonkomarev

This comment has been minimized.

Copy link
Contributor

antonkomarev commented Nov 5, 2019

I agree with @GrahamCampbell. It seems to be reasonable to have smaller components and not to pull in all utilities as single batch. Architectural changes could be addressed to 7.x then. And support package could contain many utility sub-components.

@mfn

This comment has been minimized.

Copy link
Contributor

mfn commented Nov 5, 2019

I favour the change but IMHO this shouldn't go into 6.x

Btw, I really like the name unwrapIfClosure ❤️

@iansltx

This comment has been minimized.

Copy link
Contributor Author

iansltx commented Nov 5, 2019

@mfn Why not in 6.x? This isn't a BC break; if you're relying on illuminate/support in a standalone app that uses the container, you should be requiring the package directly.

@taylorotwell taylorotwell merged commit fbdd396 into laravel:6.x Nov 6, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
@iansltx

This comment has been minimized.

Copy link
Contributor Author

iansltx commented Nov 6, 2019

Thanks for the merge, @taylorotwell!

@iansltx iansltx deleted the iansltx:remove-container-support-dep branch Nov 6, 2019
@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Nov 11, 2019

Tbh, I wouldn't make the support component any lighter. I don't see the benefit in having more components which would be support-for-x, support-for-y (just for example). I think things are fine as they are.

@iansltx good pr 👍

public function testUnwrapIfClosure()
{
$this->assertSame('foo', Util::unwrapIfClosure('foo'));
$this->assertSame('foo', UtiL::unwrapIfClosure(function () {

This comment has been minimized.

Copy link
@paulredmond

paulredmond Nov 13, 2019

Contributor

@iansltx I noticed a capital L on this line.

Actual: UtiL::unwrapIfClosure
Expected: Util::unwrapIfClosure

// @driesvints

This comment has been minimized.

Copy link
@iansltx

iansltx Nov 13, 2019

Author Contributor

Whoops.

Mind PR'ing a typo fix here, as this has already been merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.