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

[5.2] Add tightenco/collect to replace section #14118

Merged
merged 1 commit into from
Jun 23, 2016

Conversation

eFrane
Copy link
Contributor

@eFrane eFrane commented Jun 23, 2016

Since tightenco/collect is just standalone illuminate collections from support, this replace should be added.

Since tightenco/collect is just standalone illuminate collections from support, this replace should be added.
@eFrane eFrane changed the title Add tightenco/collect to replace section [5.2] Add tightenco/collect to replace section Jun 23, 2016
@GrahamCampbell
Copy link
Member

Sorry, we can't add unrelated packages to our composer file like that.

@mattstauffer
Copy link
Contributor

@GrahamCampbell Are you sure about that?

@GrahamCampbell
Copy link
Member

What I was really meaning is, I doubt we want to. Leaving for Taylor to decide. :)

@mattstauffer
Copy link
Contributor

@GrahamCampbell Thanks Graham! :)

@eFrane
Copy link
Contributor Author

eFrane commented Jun 23, 2016

how is this unrelated?

@mattstauffer
Copy link
Contributor

If you want to see reasons for and against this PR, take a look at this thread. tighten/collect#4
@frankdejonge argues against this PR; others argue for it.

@GrahamCampbell
Copy link
Member

In general third parties should not distribute laravel code like that. Laravel itself should publish this if it wanted it, surely?

@mattstauffer
Copy link
Contributor

@GrahamCampbell ¯\(°_o)/¯ I agree! But after taking that stance, Taylor and I looked into what it would take for Illuminate to offer this and decided it added a ton of unnecessary complexity to the core for a relatively edge-case need.

So, with his blessing, Tighten took on the task of splitting it out for folks who need it separately.

@eFrane
Copy link
Contributor Author

eFrane commented Jun 23, 2016

I absolutely agree with "Laravel should publish it", however since collect exists I thought this would be the right way to go.

@taylorotwell
Copy link
Member

I agree that this is probably OK in the special case of tighten/collect which is an exact copy of collections including the namespace.

@taylorotwell taylorotwell merged commit e742751 into laravel:5.2 Jun 23, 2016
@mattstauffer
Copy link
Contributor

Thanks @taylorotwell, @eFrane, and @GrahamCampbell 👍

And for future reference: if this sets a problematic precedent at any point in the future, please feel free to drop this line for the same of Laravel purity; I want this to be a service, not a burden.

@GrahamCampbell
Copy link
Member

Note that this is still not going to work.

@GrahamCampbell
Copy link
Member

You need to make the support subsplit replace it too...

@eFrane eFrane deleted the patch-1 branch June 23, 2016 16:45
@@ -68,7 +68,8 @@
"illuminate/support": "self.version",
"illuminate/translation": "self.version",
"illuminate/validation": "self.version",
"illuminate/view": "self.version"
"illuminate/view": "self.version",
"tightenco/collect": "self.version"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it just be "tightenco/collect": "*". Since any version of tightenco/collect should be ignored

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, tightenco/collect follows the version of illuminate/support, thus self.version is the correct constraint.

Copy link
Member

@crynobone crynobone Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If let say laravel/framework make a release today, and tightenco/collect are only available to run the subsplit/publish command few days/weeks (not saying it would be common, but possible). There wouldn't be any exact version available.

In this case, wouldn't both the package be installed? Or composer smart enough to validate "laravel/framework":"v5.3.0" with "tightenco/collect":"5.3.x-dev"?

@eFrane
Copy link
Contributor Author

eFrane commented Jun 24, 2016

@GrahamCampbell This may be a dumb question, but where exactly is the "authoritative" illuminate/support composer.json? Is this just the one in the illuminate/support repo or are these generated during the subsplit process?

@GrahamCampbell
Copy link
Member

The one in laravel/framework please. :)

@eFrane
Copy link
Contributor Author

eFrane commented Jun 24, 2016

Then I don't get what there is to be done for

You need to make the support subsplit replace it too...

@GrahamCampbell
Copy link
Member

Then I don't get what there is to be done for

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Support/composer.json needs updating please.

@eFrane
Copy link
Contributor Author

eFrane commented Jun 24, 2016

I'm so blind. Will do.

eFrane pushed a commit to eFrane/laravel-framework that referenced this pull request Jun 24, 2016
taylorotwell pushed a commit that referenced this pull request Jun 24, 2016
* Fix illuminate/support composer.json according to #14118

* syntax fixed
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.

5 participants