[5.8] [WIP] Extract collections from support

[mathieutu]

Hi all,

What is this?

As discussed with @taylorotwell at the Laracon and with @mattstauffer in tighten/collect#90, we need I would like to extract collections from Support package.

Why?

The main reasons are here http://mattallan.org/posts/dont-use-illuminate-support/

Actually it was almost already done by Tightenco is this package, and it works well in non Laravel projects, but due to a different namespace there are conflicts with genuine collections in Laravel projects which is extremely annoying. See tighten/collect#90 and tighten/collect#104.

How?

I've moved all the collections related classes in their own namespace/pakage, and created empty classes which extends them instead of previous ones, to avoid any breaking changes.

I'm making the PR to master (5.8) to be able to take time to do it, but actually I should not have any breaking change in it.

Still to do:

The only files I've identified as problematic to make a full isolated package are the Macroable class which can't be moved to because it's not only related to collection. I've some ideas about that but I would like to discuss to have opinions.

The question can be asked too for the contracts. For now, I've just required illuminate/contracts. It could be a proper compromise but I'm not really satisfied by this solution.

TODO list:

• Validate a name for the namespace (now Collect)
• Validate a name for the package (now illuminate/collect)
• Handle Jsonable, Arrayable contracts.
• Handle Macroable class.
• Do you see something else?

Thank's for your help,
Matt'

[m1guelpf]

@mathieutu If this is 5.8, maybe we should throw some kind of deprecation notice when using the "BC" classes?

[mathieutu]

@m1guelpf Why not, but it has to be still working, as every project use now the old namespace and the old classes.

And in fact I'm afraid that even when extending the new files in old ones, it will be a breaking change for type hinting. Maybe an alias will be necessary.
We have to pay attention to that and double check it.
If you want to help... 😛

[m1guelpf]

@mathieutu I'd say that with Shift, the upgrade guide and such we don't even need extended classes. I mean, this is a major version, you can't really expect to update the version in the composer.json and get everything working, there's a guide for that and automated tools to do it for you.

[ntzm]

I don't understand why changing namespace is needed. It can be extracted to its own package and kept in the same namespace, and avoid this breaking change.

[mathieutu]

@ntzm Each Illuminate component has its own namespace until now.
@m1guelpf This would be a maaaajor breaking change, but it will really simplify things.

I'll let the answer to @taylorotwell on theses two comments.

[ntzm]

Just because something has always been done a certain way, doesn't mean we have to continue that way. I would prefer not having a major breaking change to be honest.

[mathieutu]

@ntzm As I said I'll let Taylor state on that. BUT, in anyway I don't even see how it could be possible as it would break PSR4 standard and so composer autoloading.

[ntzm]

How would it breaking PSR-4 standard? I'm suggesting keeping it in exactly the same place, but keeping a read-only split of the collection and related classes.

[shirshak55]

I think we should go with convention & also keep repo clean. If laravel had not undergone too many breaking change and cared backward compatibility for everything we would still be in 4.x

[mathieutu]

@taylorotwell I think we need your opinion here to continue working on it.

TL;DR:

• Can it be a major breaking change? Just throwing an exception when using the old Collection/Arr classes. It'll be really easier to implement if ok.
• Todo list in first message.

[taylorotwell]

I need a better explanation of what exactly major breaking change you are referring to?

[JosephSilber]

There are over 6000 packages dependent on illuminate/support. If your user installs your package and another package dependent on illuminate/support, they need to depend on the same version or there will be a conflict. A cursory glance shows that a lot of the framework agnostic dependents are stuck on 4.1.*.

The dependencies are even worse if your package is used with Laravel or Lumen. The user can’t upgrade to the next version until you (and every other package using illuminate/support) release a new version. Now your framework agnostic package is preventing the user from updating their framework! The only alternative is to have unbounded ranges like >5.2, but that’s a really bad idea.

How would splitting it into a separate package/namespace solve that?

[crynobone]

This use data_get() which is under illuminate/support, how do you plan to make this setup work?

[crynobone]

Same here.

[crynobone]

Also use data_get().

[mathieutu]

Hi all!
Glad to have a bit of animation on this one. This is a collaborative issue, to resolve together a problem that so many people have. I don't have all the solution, this is why I've putted this issue as WIP, and waiting for comments and debate. We need to find the best compromise for all of us 😉.

@crynobone In my idea, this (all this kind of?) helper(s?) can go in the collection package. (But actually a solution I would really prefer is let the helper in support, to put this data_get "helper" in a proper class, that we can extend, in the same namespace, etc.. and make the helper just a link to this new method like other ones.)

@JosephSilber Hem. Actually It's the same for all packages in php, and all agnostic packages used by Laravel (Faker, Carbon, etc..). A different package just reduces the dependencies, and the potential conflicts between versions. tightenco/collect and many others make it right, but are not really usable due to namespace conflict and identity check in methods.

@taylorotwell the breaking change @m1guelpf would like to introduce and that would simplify the creation of the package, is to not extend at all the new classes by the old ones. The old classes will just throw some exception, and that's it. Because if we try to extend the new one, we may have some conflict problems (due to typing especially). Introduce this breaking change will directly alert people about the migration to do, without compromise.

[crynobone]

A different package just reduces the dependencies, and the potential conflicts between versions.

It doesn't solve the argument made in "Now your framework agnostic package is preventing the user from updating their framework! The only alternative is to have unbounded ranges like >5.2, but that’s a really bad idea". Package developer who depends on illuminate/collect will still need to require ~5.8 and not ^5.8 because 5.9 could still cause a breaking change.

[deleugpn]

I'm feeling like OP isn't going to solve the problem of package-agnostic dependency vs framework upgrade because that will basically happen with any package at all.
The goal seem to be that illuminate/support is too generic and there are a lot of Collection lovers out there that don't need to include the entire illuminate/support. Thinking long-term maintenance, it's theoretically easier for package-agnostic developers to deal with illuminate/collect updates than to deal with updates of illuminate/support as the amount of breaking changes is reduced.
However, I say theoretically here because in practice if you limit yourself to only using Collection in your package, then the challenge of updating your dependency will be the same since other changes to illuminate/support won't affect you at all.
At the end of the day it's only a matter of whether Laravel wants to offer Illuminate/Collect as a self-sufficient package or if it's fine to keep it inside illuminate/support.
From a broad perspective, it does feel nice to break things down into smaller chunks, but when reality kicks in there could be a maintenance nightmare when sharing features between illuminate/collect and illuminate/support, unless we make illuminate/support require illuminate/collect.

[crynobone]

Thinking long-term maintenance, it's theoretically easier for package-agnostic developers to deal with illuminate/collect updates than to deal with updates of illuminate/support as the amount of breaking changes is reduced.

Not really, for framework agnostic package project no one would want to have to release a major version (if they are following semver) every 6 months or 12 months just because Laravel decides to change some stuff with Collection.

[mathieutu]

Hey guys, thank you for your interest.

Please could we discuss about that in an another place? I would like to keep this PR quite clean and focus on technical points. The need to extract collections to another package is not to demonstrate anymore (the 1M downloads of tightenco/collect and all other similar packages is one of many proofs), we now have to figure out how to do it.

Thanks. 😉

[mattstauffer]

@mathietu It seems to me that PR’s are the place to discuss how to do the thing (which you are suggesting) but also whether to do the thing and why, which is what the other folks are discussing. There’s a reason tightenco/collect exists: Taylor and I did a proof of concept of something similar to this PR and decided it was too much burden on illuminate. So I split collect.

I hope we can make this work and retire future development of collect. But I don’t think we should halt the conversation yet of whether this is even wise or better. Just my two cents tho. :)

[deleugpn]

I'm also trying not to flood this channel, but as someone who never suffered from this it's really hard to understand what problem this is solving.

[taylorotwell]

Honestly just going to table this for now. Reason being:

It's likely that we will be removing most of the global Arr and Str helpers from support/helpers.php which will drastically reduce the amount of global functions. Others may be able to eventually move out as well.

There isn't an obvious path for how to do this without headache.

If people want to use a collection package and they don't need illuminate/support there are already others that are framework agnostic out there they could pull into their project.

[mathieutu]

@taylorotwell This is actually the problem, the "framework agnostic" ones are not compatible with Laravel and lead to conflicts with original collections.

There is a true demand for extracting them (I let you see the number of version of collection forks), and actually not a lot of work to do. Just some decisions to take, and you're the only one capable of taking them.

Anyway, someone will maybe reopen the subject in a next version.