Skip to content

Conversation

@Kriso1337
Copy link

change for compatibility with external redis manager (e.g. sentinel library in my case)

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Is you sentinel RedisManager implementation open-source or from a package?

Comment on lines 63 to 64

$this->container()->afterResolving(RedisManager::class, static function (RedisManager $redis): void {
$this->container()->afterResolving(RedisFactoryContract::class, static function (RedisFactoryContract $redis): void {
$redis->enableEvents();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kriso1337
Copy link
Author

Kriso1337 commented Dec 5, 2024

Is you sentinel RedisManager implementation open-source or from a package?

from package, it sounds like we can omit RedisManager type(and then add it to phpdoc)?

@stayallive
Copy link
Collaborator

Thanks for linking the package, looks like this will work for that specific package.

it sounds like we can omit RedisManager type(and then add it to phpdoc)?

I don't understand what you mean with this? Can you elaborate?

@Kriso1337
Copy link
Author

Thanks for linking the package, looks like this will work for that specific package.

it sounds like we can omit RedisManager type(and then add it to phpdoc)?

I don't understand what you mean with this? Can you elaborate?

sure, here it is, with this realization it pass any RedisManager, default and any with implemented contract, so we can use custom manager

or maybe I didn't understand you and you are worried about another problem?

@stayallive
Copy link
Collaborator

stayallive commented Dec 8, 2024

So the main concern is that Sentry is installed in many applications and if we ask to container to give us an object of the Illuminate\Contracts\Redis\Factory type it can be any object. Not only the default Laravel one or the one from the Sentinel package. So it isn't guaranteed that enableEvents is there to call. In fact the contract guarantees it is not, there is no contract that specifies the enableEvents method is present.

Since there is no real good workaround this we are not making this change at this time. You can add the following snippet to your own AppServiceProvider which has the same effect and the Sentry SDK will be able to use the events to show the Redis operations as if it enabled the events itself:

$this->app->afterResolving(\Monospice\LaravelRedisSentinel\RedisSentinelManager::class, static function (\Monospice\LaravelRedisSentinel\RedisSentinelManager $redis): void {
    $redis->enableEvents();
});

@stayallive stayallive closed this Dec 8, 2024
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