getsentry / sentry-laravel

The official Laravel SDK for Sentry (sentry.io)
https://sentry.io
MIT License
1.25k stars 189 forks source link

Is there any reason why Hub/HubInterface is not bound? #248

Closed SCIF closed 5 years ago

SCIF commented 5 years ago

Hi there,

I cannot use constructor injection of HubInterface and Hub and have to add:

        $this->app->when(MyService::class)
            ->needs(HubInterface::class)
            ->give(
                function (): HubInterface {
                    return $this->app->get('sentry');
                }
            );

Having several instances of Sentry sounds not really correct for me, since different services add breadcrumbs. Moreover, you sentry works as singleton. Why not bind interface as well? I assume there should be some drawback but have no idea which one is it.

Thanks!

stayallive commented 5 years ago

We do this to only call Hub::setCurrent once in the application lifecycle.

In the boot method of the service provider we "force" the singleton to be resolved making Sentry and it's error handling available as soon as possible (for error tracking).

The Hub is not in the container since it's a singleton and can be used by the user to get to the current Sentry client or set a new one / do stuff with the scopes and breadcrumbs for the global Sentry client.

The choices made in the base SDK are made because at Sentry we try to implement a Unified API across languages. You can read more about that here: https://docs.sentry.io/development/sdk-dev/unified-api/.

Could you tell me a bit what you are trying to achieve so I can see if there maybe is another way to do what you want to do?

SCIF commented 5 years ago

Hi @stayallive,

Thanks for the answer. Yes, I can see and mentioned that sentry is singleton, but it's not really clear for me why HubInterface is not bound to sentry.

I have several services using Sentry injected by constructor. Such services adds vital breadcrumbs simplifying understanding what happened. For instance:

final class DatabaseManager
public function __construct(DatabaseManager $manager, HubInterface $sentry)
{…

This class adds breadcrumbs of changing database, etc

Some services like ImageUploader catches exceptions and reports them gracefully.

SCIF commented 5 years ago

At the moment I've created just:

$this->app->bind(HubInterface::class, 'sentry');

And it works fine for me.

stayallive commented 5 years ago

Ah! Well now it makes way more sense, I totally missed your point, sorry about that 😅

Well, I would be more than happy to accept a PR that adds that line in the correct place 😉

SCIF commented 5 years ago

Cool! I'll do that! :)

SCIF commented 5 years ago

Unfortunately, I was not able to check if it works in Lumen. Hopefully, app has the same alias() method as Laravel's one.