Skip to content

give newbees a package service providers section, preventing adding after application #3855

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

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

BartHuis
Copy link
Contributor

@BartHuis BartHuis commented Aug 4, 2016

when i started, i ended up putting all package specific service providers just at the bottom of the application service providers. But when making a route group like domain.com/{keyword}, the packages that are registering its own url's, not work anymore (like now when i installed the translations manager from barryvdh). When putting them above the app specific, everything works. So just give it to the user as a default place?

…fter application

when i started, i ended up putting all package specific service providers just at the bottom of the application service providers. But when making a route group like domain.com/{keyword}, the packages that are registering its own url's, not work anymore (like now when i installed the translations manager from barryvdh). When putting them above the app specific, everything works. So just give it to the user as a default place?
@BartHuis
Copy link
Contributor Author

BartHuis commented Aug 4, 2016

why is this check failed? :S i didn't change any code, only comments...

@GrahamCampbell
Copy link
Member

There's still cs rules there. Don't worry about it though. StyleCI will auto-fix after merge.

@BartHuis
Copy link
Contributor Author

BartHuis commented Aug 4, 2016

Ah, thanks @GrahamCampbell :) will you be merging my suggestion? ;)

@GrahamCampbell
Copy link
Member

Up to Taylor to decide. :)

@brayniverse
Copy link
Contributor

I always add this myself, though, I typically call it "Third Party Service Providers", nonetheless, it would be nice to see this merged.

@BartHuis
Copy link
Contributor Author

BartHuis commented Aug 4, 2016

yeah, thats good for me too @brayniverse

@GrahamCampbell
Copy link
Member

On a side note, the first thing I do is delete these comments. :trollface:

@BartHuis
Copy link
Contributor Author

BartHuis commented Aug 4, 2016

hahaha

@BartHuis
Copy link
Contributor Author

BartHuis commented Aug 4, 2016

that's your choice ;)

@barryvdh
Copy link
Contributor

barryvdh commented Aug 4, 2016

I do this also, and I leave the comments ;)

But yeah 👍 for this, especially prevents problems with a catch-all route in your own Provider.

@brayniverse
Copy link
Contributor

@GrahamCampbell haha, well I guess it's down to personal taste.

@taylorotwell taylorotwell merged commit 14f79f1 into laravel:master Aug 4, 2016
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