Skip to content

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