monospice / laravel-redis-sentinel-drivers

Redis Sentinel integration for Laravel and Lumen.
MIT License
101 stars 48 forks source link

Lumen compatibility #8

Closed lorique closed 7 years ago

lorique commented 7 years ago

There is a lumen compatibility issue with v2.0.0. Error as follows:

lumen.ERROR: Symfony\Component\Debug\Exception\FatalErrorException: Access level to Monospice\LaravelRedisSentinel\RedisSentinelManager::resolve() must be public (as in class Illuminate\Redis\RedisManager) in /var/www/oase-favorites/vendor/monospice/laravel-redis-sentinel-drivers/src/RedisSentinelManager.php:29
Stack trace:
#0 /var/www/oase-favorites/vendor/laravel/lumen-framework/src/Concerns/RegistersExceptionHandlers.php(54): Laravel\Lumen\Application->handleShutdown()
#1 [internal function]: Laravel\Lumen\Application->Laravel\Lumen\Concerns\{closure}()
#2 {main} 

To fix, RedisSentinelManager->resolve($name) must be public, and $name be optional.

-    protected function resolve($name)
+    public function resolve($name = null)

v2.1.0 is not compatible with lumen, as the illuminate\foundation package is required, and it injects a ton of BS into lumen you simply don't need. You might as well run Laravel at that point. Can we have a v2.0.1 with the above fix in?

cyrossignol commented 7 years ago

Thanks @lorique, I should have a fix out for this sometime this week... made a mistake by coupling the code to full Laravel. Closing this one...we'll keep track of the Lumen issue in #7.

cyrossignol commented 6 years ago

@lorique, support for Lumen 5.4+ is up on the 2.x branch (2.x-dev). Just need to update all the documentation that changed before releasing this.

I added optional support for environment-based config to adhere to Lumen's configuration model. I'm sure you already have your configs set up for your projects, but I'd appreciate any feedback you may have before this is released. The package's internal config file contains hints you can use before the documentation is finished:

https://github.com/monospice/laravel-redis-sentinel-drivers/blob/2.x/config/redis-sentinel.php

By the way, sorry I missed the fact that you were using Lumen in the last issue you submitted (#5)!

Stoffo commented 5 years ago

Sorry to bring that old issue back up, but I try to use this package with lumen 5.4.7 and still get the following error:

In Laravel540RedisSentinelManager.php line 19:

  Access level to Monospice\LaravelRedisSentinel\Manager\Laravel540RedisSentinelManager::resolve() must be public (as in class Illuminate\Redis\RedisManager)

I'm using version 2.5.0 of this package. Am I doing something wrong?

Thanks in advance!

cyrossignol commented 5 years ago

@Stoffo Let's try to find out what's happening. We can check the Lumen framework version that your application is actually running by adding some debug code to bootstrap/app.php. Add this right after the file instantiates a new instance of Laravel\Lumen\Application:

dd($app->version());

For version 5.4.7, we should see:

Lumen (5.4.7) (Laravel Components 5.4.*)

This package only loads the Laravel540RedisSentinelManager for Lumen versions earlier than 5.4, because Lumen typically installs the latest "Illuminate" components for that major version. It checks the same version that we dumped above:

$version = substr($app->version(), 7, 3); // "5.4" 
$isOld = version_compare($version, '5.4', 'lt'); // false 

In other words, the package uses Laravel540RedisSentinelManager for Lumen 5.3 an below. If the version string doesn't match what I wrote above, we'll need to adjust the package's Lumen version detection strategy.

Stoffo commented 5 years ago

@cyrossignol thank you so much! That was the problem right there, the version string was overriden in my application, resulting in this bug.

So there is no problem with the version detection in the package😄

cyrossignol commented 5 years ago

@Stoffo Glad to hear it! Thanks for checking.