Hello,
It's the first time I really use a service provider.
I have written this one.
class DatasourceServiceProvider extends ServiceProvider
{
/**
* Register services.
*/
public function register(): void
{
$this->app->singleton(DatasourceInterface::class, function (Application $app) {
$user = auth()->user();
$datasource = $user->company->datasource?->value;
if ($datasource) {
switch ($datasource) {
case 'pipedrive':
return new PipedriveDataService();
case 'googlesheet':
return new GoogleSheetDataService();
default:
throw new Exception('Unsupported data source', 404);
}
}
});
}
/**
* Bootstrap services.
*/
public function boot(): void
{
//
}
}
And the related service.
class PipedriveDataService implements DatasourceInterface
{
private $url;
private $token;
public function __construct()
{
$this->url = config('services.pipedrive.api_url');
$credentials = auth()->user()->company->datasource_credentials;
$this->token = $credentials['pipedrive_token'];
}
public function getUsers()
{
$users = null;
$response = Http::withQueryParameters([
'api_token' => $this->token,
])->get($this->url.'users');
if ($response->successful()) {
$users = Arr::map($response->object()->data, function (object $user, string $key) {
return (object) [
'source_id' => $user->id,
'name' => $user->name,
'email' => $user->email,
];
});
}
return $users;
}
}
Is it a good idea to use auth()->user() inside the service ? Or is it better to inject the user directly in the service provider ?
Furthermore I need the credentials in each service, is it a good idea to retrieve them in the service or would it be better to retrieve them in the service provider and pass them in the service constructor ?
Other advice ?
Thanks for your suggestions.
V
keep in mind that auth()->user() will be null outside the context of http requests, ex when the app is booted by crontab to perform a scheduled command, booted by supervisor to perform a queued job or you are running something from the cli
@vincent15000 Service providers are used to register services in the container. However, you don’t really seem to be taking advantage of this as your class is still “asking” for things rather than having dependencies provided.
For example, your PipedriveDataService is asking for a configuration value, and then using a helper to magically get the authenticated user, instead of these dependencies being given to the class. And as @krisi_gjika points out, this will also fail in non-HTTP contexts where there isn’t a notion of an authenticated user.
Instead, you should be making these things arguments in your class’s constructor, and resolving values for those parameters and providing them in your service provider:
class PipedriveDataService implements DatasourceInterface
{
protected string $url;
protected User $user;
protected string $token;
public function __construct(string $url, User $user)
{
$this->url = $url;
$this->user = $user;
$this->token = $this->user->company->datasource_credentials['pipedrive_token'];
}
}
public function register(): void
{
$this->app->singleton(DatasourceInterface::class, function (): void {
$user = $this->app->make('auth')->user();
return match ($user->company->datasource->value) {
'pipedrive' => new PipedriveDataService(
url: $this->app->make('config')->get('services.pipedrive.api_url'),
user: $user,
),
};
});
}
The PipedriveDataService now requires a URL and a user to be instantiated, and the service provider resolves and passes those arguments. Your class is no longer using helpers to magically grab values from somewhere that may return null or not be set at all.
@martinbean Ok thank you very much for your example.
@martinbean What's the difference between those two lines ?
$user = auth()->user();
$user = $this->app->make('auth')->user();
@vincent15000 Where they’re being used is the biggest difference.
You used auth()->user() to try and resolve the user inside the class itself. In my example, I resolve the user in the service provider and then pass it as an argument to the class, so the class is given its dependencies instead of just trying to magically grab them using helpers or similar.
This is what dependency injection is in a nutshell: give classes the things they need, instead of those classes calling out and trying to fetch them from outside the class. Trying to grab stuff using a helper or a facade is usually a strong indication you should probably be injecting a dependency somewhere instead.
@martinbean while I do agree that you should not get dependencies out of helper methods inside the class itself, you should also not do the same in the service provider.
@vincent15000 since you need to resolve an implementation conditionally, sounds like what you are looking for is a factory. you can still bind your factory to the container if you want, but the dependency of user should be resolved at the moment the implementation is resolved with the full context of your code, not when the application boots up with little to no context. Here is a simple example, but you can also make the factory a class if you really want to typehint it.
$this->app->singleton('datasourceFactory', function (Application $app) {
return function (Company $company) {
return match ($company->datasource->value) {
'pipedrive' => new PipedriveDataService(
url: $this->app->make('config')->get('services.pipedrive.api_url'),
company: $company,
),
... // handle other cases as well as the default case
};
});
instead I have bound the factory itself to the container, now it makes more sense to have a singleton and you can use the factory in any context:
class UserController extends Controller
{
public function show(Request $request, User $user)
{
/** @var DataSourceInterface $source */
$source = app('datasourceFactory')($user->company);
...
class DoStuffCommand extends Command
{
public function handle()
{
Company::query()->each(function (Company $company) {
/** @var DataSourceInterface $source */
$source = app('datasourceFactory')($company);
});
while I do agree that you should not get dependencies out of helper methods inside the class itself, you should also not do the same in the service provider.
@krisi_gjika I didn’t? I instead resolved the auth service, and called the user from that, whilst also pointing out the caveat that will fail if called in a non-HTTP context where there isn’t going to be an authenticated user at all.
Please or to participate in this conversation.