-
Notifications
You must be signed in to change notification settings - Fork 251
added proper config support #15
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
Conversation
|
This breaks previous insyallations |
|
okay.... well I can add a check for the old configuration file as well. Will do it later when I get time. |
|
That should do it |
|
Also, try not to mix $app['config'] syntax with the Facade pattern. My big concern and the concern that @jeremeamia expressed was that this breaks the test cases. Maybe @taylorotwell can sprinkle some testing knowledge? |
|
I don't think facade syntax should be used within library code at all. Facade usage should be limited to application code, IMO. If need to tag these config changes for 1.1, that is fine. However, I won't have a very high level of confidence that this is correct unless everyone agrees that it is the correct Laravel way to do it and if I have passing tests. I'd like Ryan and Taylor to both approve the PR before I take it. Thanks for your work on this! |
|
Bumping the version number isn't the issue. The problem is testability. We need to switch the tests to facilitate the registration as a package. |
|
Also, how is this PR different from #8. @iricketson was trying to do the same thing, but also didn't have passing tests. It looks like these two PRs are attempting to fix the same issue. Is one of them doing something better than the other? |
|
Thanks for your work on this, but I'm closing this in favor of #17 which has more complete testing around the changes. |
Added support for php artisan config:publish aws/aws-sdk-php-laravel