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

Update service provider to use bindShared(). #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update service provider to use bindShared(). #21

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 2, 2014

Possible changes to the Laravel service provider.

Warning: not tested.

$config->addConnection('default', $this->config);
$config->addConnection('default', [
'dbname' => $credentials['database'],
'user' => $credentials['username'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the isset checks in the original code here due to the sqlite::memory: connection not needing a username, password, or host. Can you add them back in?

The default laravel configuration does not define all config options for
the sqlite connection:

https://github.com/laravel/laravel/blob/master/app/config/database.php

Add an `isset()` check on `$credentials` for the following index to avoid
PHP undefined index notices:

$credentials['username']
$credentials['password']
$credentials['host']
@harikt
Copy link

harikt commented Sep 3, 2014

In my humble opinion, the service providers should stay on a separate repo. It should not be part of the library else you will end up with all frameworks :) .

@ghost
Copy link
Author

ghost commented Sep 3, 2014

It's not a bad idea.

AWS does this for instance: https://github.com/aws/aws-sdk-php-laravel

@harikt
Copy link

harikt commented Sep 3, 2014

@travis-rei yup 👍

@vlucas
Copy link
Collaborator

vlucas commented Sep 3, 2014

True - it would be much cleaner that way.

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.

3 participants