Skip to content

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

Closed
wants to merge 4 commits into from
Closed

added proper config support #15

wants to merge 4 commits into from

Conversation

andheiberg
Copy link

Added support for php artisan config:publish aws/aws-sdk-php-laravel

@rtablada
Copy link

This breaks previous insyallations

@andheiberg
Copy link
Author

okay.... well I can add a check for the old configuration file as well. Will do it later when I get time.

@andheiberg
Copy link
Author

That should do it

@rtablada
Copy link

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?

@jeremeamia
Copy link
Contributor

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!

@rtablada
Copy link

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.

@jeremeamia
Copy link
Contributor

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?

@jeremeamia jeremeamia mentioned this pull request Aug 30, 2013
@jeremeamia
Copy link
Contributor

Thanks for your work on this, but I'm closing this in favor of #17 which has more complete testing around the changes.

@jeremeamia jeremeamia closed this Sep 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants