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

Support Laravel 5.2 #215

Closed
wants to merge 4 commits into from
Closed

Conversation

EspadaV8
Copy link
Contributor

Laravel 5.1 deprecated using bindShared and 5.2 removed it completely, because of this a package update to cocur/slugify is needed to at least 1.4. orchestra/testbench 3.0 only supports Laravel 5.0.* so that requires an update as well.

Illuminate\Foundation\Composer has been moved to Illuminate\Support\Composer

@EspadaV8
Copy link
Contributor Author

The failed test is because of PHP 5.4 which isn't supported by Laravel 5.2 so the change in the Composer class location throws an error.

@cviebrock would you be okay with me updating composer and travis to remove PHP 5.4?

@EspadaV8
Copy link
Contributor Author

Dropping PHP 5.4 would also mean increasing the minimum Laravel version to 5.1 (which requires PHP 5.5.9 or above).

@EspadaV8
Copy link
Contributor Author

Actually, it's going to increase Laravel to 5.2 anyway isn't it, because it requires the Composer class in that location. Duh.

@nivv
Copy link

nivv commented Dec 23, 2015

@EspadaV8 I saw this commit from another repo:

https://github.com/laracasts/Laravel-5-Generators-Extended/pull/83/files

That would (allegedly, since I haven't tested it) make it work on both 5.1 and 5.2 (including the things you proposed)

@EspadaV8
Copy link
Contributor Author

That requires use of the global app function. Does this package mind requiring that? It would also be possible to inject the Container class and use that to resolve the Composer class instead of using the global function, which might be a bit nicer.

@nivv
Copy link

nivv commented Dec 23, 2015

@EspadaV8 agreed! Lets see what @cviebrock thinks 🎅

@EspadaV8
Copy link
Contributor Author

Just had a look at the support report and it doesn't contain the Container class so either was the package would need to be updated with (I'm guessing) foundation added as a requirement. (on my mobile so can't check easily).

I would probably recommend tagging a final 0.1x release and then release a new 0.2x for L5.2 and up.

@EspadaV8
Copy link
Contributor Author

Hmm, okay, so the Container can't be injected because Commands don't get passed through the IoC and only SluggableMigrationCreator and an instance of Composer get passed in.

Using app()['composer'] works, but I'm not a great fan of using the global function.

What can be done though, is to remove the second parameter from the constructor and then use func_get_arg(1) to get the composer instance. I'm still not a great fan of this approach either.

@hiendv
Copy link

hiendv commented Dec 24, 2015

@EspadaV8 So should we change the namespace on a new release or using func_get_arg?

@cviebrock
Copy link
Owner

This PR looks good, but probably warrants a version bump. Let me take a look it all more closely in the next few days.

I wanted to do a major refactor of things for the next version, but I might not get to that in time, and I don't want to make things unusable for Laravel 5.2 users.

@hiendv
Copy link

hiendv commented Dec 25, 2015

We should have a branch for 5.2 or using IoC for resolving Composer so that it can be compatible with 5.*

@cviebrock
Copy link
Owner

So, I think there are now several items that need resolving for 5.2 compatibility.

First is the composer issue. Is there an injection we could make for Composer that works in 5.1 and 5.2? If so, let's use that. If not, I'd almost be tempted to just entirely remove the use of composer in that class, and not do the ->dumpAutoloads(), rather than come up with version-specific workarounds.

Second issue is the bindShared vs singleton. If we can just upgrade cocur/slugify to 1.4, then that's fine with me.

Finally, updating workbench and travis to support 5.2. It doesn't look like we can do this and make things compatible with 5.2 and earlier 5.* versions, does it? So, probably a new branch (and major version bump) are going to be needed anyway.

Sometime this holidays, I'll deal with it. :)

@cviebrock cviebrock mentioned this pull request Dec 25, 2015
@cviebrock
Copy link
Owner

Try the 3.1.4 release.

I removed the composer reference to make it compatible with Laravel 5.2, and fixed composer.json so that it should work with all 5.x versions.

@cviebrock cviebrock closed this Jan 3, 2016
@Alvarad0
Copy link

Como puedo desinstalar la version mas reciente de Sluggable para instalar la version 3.1.4

@cviebrock
Copy link
Owner

Don't use 3.1.4. Use 4.0.1 now.

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.

None yet

5 participants