Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

Consider changing namespace #54

Closed
adamwathan opened this issue Sep 14, 2017 · 7 comments
Closed

Consider changing namespace #54

adamwathan opened this issue Sep 14, 2017 · 7 comments

Comments

@adamwathan
Copy link

Right now there's a weird situation that can occur where you get two versions of Illuminate\Support\Collection installed in your project but can only access the Laravel version.

Say you have a Laravel 5.1 project, where the collection class doesn't have a function like mapWithKeys.

If you run composer require "tightenco/collect:^5.5", this package's version of the Collection classes will be installed successfully.

But if you try Collection::make([1, 2, 3])->mapWithKeys(...), you'll get an error that there is no mapWithKeys method, because Composer happens to load Laravel's version of the class before the Tighten version.

This becomes a more obvious problem when someone tries to require a package that depends on tightenco/collect, rather than tightenco/collect itself.

For example, Zttp requires tightenco/collect:^5.4. Currently, if someone tries to install Zttp into a Laravel <=5.3 project, Zttp just doesn't work, because it can only access the Laravel version of Collection, which is an outdated version.

Switching the namespace to TightenCo/Collect would resolve this by allowing packages to depend on the Tighten version and reference it explicitly, instead of silently accessing the wrong class because Composer happens to load it first.

This is sort of a pain in the ass issue since it's ideal for the tightenco/collect version number to match Laravel's version number, so not sure how to deal with it :/

@mattstauffer
Copy link
Member

@adamwathan I have been thinking about this for a long time. I think it's almost definitely the right decision. And I think it's fine to make the version number match, since we're doing that manually right now anyway.

I want to ask for any suggestions for why this could cause any harm, but I've been thinking this direction for a while so I'm definitely ready for us to start considering it--the conflicts between packages requiring collect and apps that already have illuminate support are the biggest problem area here for sure.

@adamwathan
Copy link
Author

The only issue I can see is that we'd need to wait for a new version of Laravel to be tagged to tag a new release of this package with the changed namespace in order to keep the versions matching, but that new version will be a patch release and it'll be a major BC break.

One possible solution is to duplicate the entire Illuminate folder in this package and provide all of the same code under two namespaces, so someone using this package can choose to use the new namespace without breaking anything for people relying on the Illuminate namespace.

@RemiCollin
Copy link

I'm wondering if it wouldn't be possible to use a conditionnal class_alias() to change namespace and provide backward compatibility at the same time. Something like :

if (! class_exists(Illuminate\Support\Collection::class) {
    class_alias(TightenCo\Collect\Collection::class, Illuminate\Support\Collection::class;
}

That way the package would silently defer to actual Illuminate\Support\Collection implementation if it's already in the dependency tree. Didn't try something like this before but I guess it's worth a try.

That said, the best solution would be a Illuminate\Collection package.

@damiani
Copy link
Contributor

damiani commented Sep 15, 2017

One potential problem with this approach: functions in helpers.php from the first package loaded will still take precedence, so things like collect() and with() could still make unpredictable references to the "wrong" package.

@adamwathan
Copy link
Author

Yeah I think that's unavoidable though; I know as a package maintainer myself I will just immediately switch to using TightenCo/Collection::make() to be explicit and avoid any ambiguity.

@damiani
Copy link
Contributor

damiani commented Sep 15, 2017

Yeah I guess that's a fair trade-off, better than blowing everything up.

@mathieutu
Copy link

@adamwathan I though like you about Collection::make() but actually it doesn't work. Check my issue about that: #90

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants