search

codestudiohq / laravel-totem

Manage Your Laravel Schedule From A Web Dashboard
MIT License
1.79k stars 224 forks source link

TotemServiceProvider should not make database connection check during register() #305

Closed cyruscollier closed 2 years ago

cyruscollier commented 3 years ago

https://github.com/codestudiohq/laravel-totem/blob/8.0/src/Providers/TotemServiceProvider.php#L57

if (! defined('TOTEM_DATABASE_CONNECTION')) {
    define('TOTEM_DATABASE_CONNECTION', config('totem.database_connection', Schema::getConnection()->getName()));
}

This forces a database connection check during the Laravel bootstrapping process, which affects package discovery, composer install, all artisan commands, etc. This in turn breaks the application when attempting to do things like run composer or artisan commands while not in a real environment with a database connection, such as in a CI container or in a local terminal if using vagrant/docker/etc. for the actual local server. I'm sure there are other cases where this would break as well, but these examples are what I experienced in my own workflow.

Laravel's documentation on this matter states that a Service Provider's register() method "should only bind things into the service container. You should never attempt to register any event listeners, routes, or any other piece of functionality within the register method. Otherwise, you may accidentally use a service that is provided by a service provider which has not loaded yet." While this is not technically a service provider load order issue, attempting to connect to the database clearly still falls in the category of functionality not related to service container binding.

I believe the same behavior can be achieved without connecting to the database by relying solely on config:

if (! defined('TOTEM_DATABASE_CONNECTION')) {
    define('TOTEM_DATABASE_CONNECTION', config('totem.database_connection', config('database.default'));
}

I'm happy to submit a PR if this seems like the right solution.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cyruscollier commented 2 years ago

Any response to this?

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cyruscollier commented 2 years ago

Any response to this?

qschmick commented 2 years ago

@cyruscollier Feel free to open the PR

cyruscollier commented 2 years ago

Pull request submitted. Thanks!