Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.0] Redis Breaking Change not documented #29864

Closed
LKaemmerling opened this issue Sep 5, 2019 · 39 comments
Closed

[6.0] Redis Breaking Change not documented #29864

LKaemmerling opened this issue Sep 5, 2019 · 39 comments

Comments

@LKaemmerling
Copy link

  • Laravel Version: 6.0.0
  • PHP Version: 7.3.9
  • Database Driver & Version:

Description:

With 39699b8 there was introduced a breaking change. This creaking change is not documented anywhere within the upgrade guide.

Steps To Reproduce:

I have prepared a repository containing the code to reproduce this behavior. There is also a thread where i wrote a little bit with @driesvints about this:
https://twitter.com/Lukas_Kae/status/1169314825006145536

The repository:
https://github.com/LKaemmerling/laravel-6-redis-bc

  1. Setup a Forge Server (PHP 7.3, MySQL 5.7)
  2. Install this repo as a site (or any other laravel setup with just horizon installed)
  3. Add Flare/Sentry/Whatever Credentials to your .env
  4. Start a daemon which is running php artisan horizon
  5. Add php artisan horizon:terminate to the Forge deployment script
  6. Deploy
  7. 💥 The deployment should fail with Please remove or rename the Redis facade alias in > your "app" configuration file in order to avoid collision with the PHP Redis extension. (https://flareapp.io/share/317xd17b#F24)
  8. Rename the Redis-Facade to RedisManager (maybe directly on the server)
  9. Restart the daemon from Step 4
  10. Deploy
  11. 💥 The deployment should fail with Class 'Redis' not found (https://flareapp.io/share/wx7K3K7R#F22)
  12. Install php-redis (sudo apt install php-redis)
  13. Deploy
  14. Now everything should work.
@driesvints
Copy link
Member

What's your REDIS_CLIENT setting?

@LKaemmerling
Copy link
Author

The REDIS_CLIENT setting is the default one from : https://github.com/LKaemmerling/laravel-6-redis-bc/blob/master/config/database.php#L122

The steps from above are the steps which are nessacarry to trigger the problem. I didn't change anything else.

@driesvints
Copy link
Member

I think in this case the problem is that php-redis isn't installed on your server. There isn't a breaking change. You're setting the driver to phpredis which uses the C extension ext-redis so the driver won't work without the extension (hence it can't find the Redis class from PHP).

It's indeed a problem that Forge doesn't installs this. I'll check in on this with @themsaid.

@LKaemmerling
Copy link
Author

The problem is that this change is not documented in the Upgrade Guide, there should be a notice about this change...

@driesvints
Copy link
Member

@LKaemmerling but this isn't an issue with Laravel. The fact that the default was changed doesn't matter. The problem here is that you need the ext-redis extension in order to make the phpredis driver to work.

@driesvints
Copy link
Member

That's also documented in the introduction here: https://laravel.com/docs/6.0/redis#introduction

@driesvints
Copy link
Member

We're looking into updating Forge for this to install ext-redis by default btw.

@kreitje
Copy link
Contributor

kreitje commented Sep 5, 2019

Thanks Dries!

This one definitely caught me too. I will look at a PR for the miscellaneous section of the upgrade guide calling it out. That is where it references if you want to keep your files in sync with laravel/laravel which is where this is coming from.

@driesvints
Copy link
Member

There's nothing about this in the upgrade guide because no breaking was made. The default was changed in the skeleton so only new apps should have the new default (and thus need to install ext-redis).

@fitztrev
Copy link
Contributor

fitztrev commented Sep 5, 2019

The default was changed in the skeleton so only new apps should have the new default

It was changed in the framework, too.

https://github.com/laravel/framework/pull/29745/files#r317621260

If someone doesn't have a redis.client key/value pair in their config, I think it would be breaking because this would now fallback to phpredis.

@sebdesign
Copy link
Contributor

I experienced the same issue with an existing application:

I'm using the predis/predis package, and not the PHP extension, since that was easier to set up.
In Laravel 5.x there was no need to set the REDIS_CLIENT because the default value of the database.redis.client was predis, which worked as expected with the predis/predis package.
Using the Github comparison tool I changed (without much thinking) the default value from predis to phpredis, and then I never thought to add REDIS_CLIENT=predis to my .env file.

Upon deployment this exception occured, which is quite misleading, because actually I shouldn't remove/rename the alias, but instead change the configuration value to predis or add the environment variable to my .env.

@Arkanius
Copy link
Contributor

Arkanius commented Sep 5, 2019

I just can't understand how this is a breaking change. Look, in your previous laravel versions the default value of client at config/database.php is predis.

At newst versions the default value changed to phpredis with REDIS_CLIENT env, but your apps should work normally

@fitztrev
Copy link
Contributor

fitztrev commented Sep 5, 2019

@Arkanius The database.redis.client config value was added in 5.4 and has technically been optional. Before 6.0, if not found, it would fallback to predis. But in 6.0, the fallback was changed to phpredis.

@driesvints
Copy link
Member

If someone doesn't have a redis.client key/value pair in their config, I think it would be breaking because this would now fallback to phpredis.

But that only happens if you update your config manually.

Using the Github comparison tool I changed (without much thinking) the default value from predis to phpredis, and then I never thought to add REDIS_CLIENT=predis to my .env file.

But you're saying it here: you changed the default. If you change a default value then things can't keep working as expected now can they? 😅

At this point I'm inclined to add a note about the upgrade guide about the change but there really isn't any breaking change. Feel free to send in a PR to the docs.

@driesvints
Copy link
Member

@fitztrev that might indeed be a more noteworthy thing to add to the upgrade guide. Feel free to send in a PR. Otherwise I'll try to do so tomorrow.

@fitztrev
Copy link
Contributor

fitztrev commented Sep 5, 2019

@driesvints 👍 See laravel/docs#5448. Edits welcome.

@mycarrysun
Copy link

@sebdesign that's exactly what happened to me as well. A notice in the upgrade guide really helps especially when at the bottom of the guide it says to use the Github comparison tool. I didn't think it would try to connect to the redis client without me using it, since I only use the redis client for cache and not a database. Maybe it also should only try to connect when the redis database connection is requested?

@driesvints
Copy link
Member

This was added to the upgrade guide.

@kreitje
Copy link
Contributor

kreitje commented Sep 23, 2019

config/database.php would have remained predis unless you changed it when going through and updating your files to match laravel/laravel:6. That's why it might not be considered a breaking change. As a developer, I had to update the default to cause this break.

@swayok
Copy link

swayok commented Oct 3, 2019

I had no 'databse.redis.client' configured explicitely and after upgrade to Laravel 6 got tons of surprises because of it...

@mfn
Copy link
Contributor

mfn commented Oct 3, 2019

I had no 'databse.redis.client' configured explicitely

I've seen this also, it happens if you have started with a Laravel version < 5.4.0, at which point the setting didn't existed, and upgraded over the years.

Unless you manually synced the changes over from https://github.com/laravel/laravel or republish the configs, you're gonna miss out some of the changes over time.

@rayblair06
Copy link

I too solved this issue by running sudo apt install php-redis on the server.

The problem was once I changed and pushed my code with phpredis, my application was locked with that error. I suppose you could manually remove the config cache and push with the default as predis.

@newalphamedia
Copy link

I can't seem to resolve this issue myself. I've setup a new site and removed predis and installed php-redis (note that sudo apt install php-redis isn't working for some reason - it returns 404 error) however I then discovered that php directories are missing a "20-redis.ini" file which would enable the php-redis extension. Not sure how to properly resolve this - do I need to reinstall my homestead/vagrant setup to get everything updated?

@gauravmak
Copy link
Contributor

@driesvints Installing in Forge provisioned servers by default?

@driesvints
Copy link
Member

@gauravmak as of recently ext-redis is installed by default yeah.

@kreitje
Copy link
Contributor

kreitje commented Oct 17, 2019

@newalphamedia try doing sudo apt-get update then installing php-redis.

@newalphamedia
Copy link

@kreitje thank you very much - that worked!

For anyone else wanting to resolve this issue, you may try running these commands to on Homestead to get php-redis working:

  1. vagrant ssh
  2. sudo apt-get update
  3. sudo apt install php-redis
  4. Go to app/config/database.php and change value from "predis" to "phpredis" for value "REDIS_CLIENT" (not always required)
  5. Go to app/config/app.php and rename Predis to PredisManager under Aliases
  6. sudo service php7.3-fpm restart (replace "7.3" with whatever version of PHP as required - you can check with command "sudo php -v")

That should be it! Worked for me.

@mfn
Copy link
Contributor

mfn commented Oct 18, 2019

Also, for enabling it "the Debian way, you can use:
sudo phpenmod redis ; this ensures the proper 20-redis.ini file is in place (symlinked)

@asterism612
Copy link

@kreitje thank you very much - that worked!

For anyone else wanting to resolve this issue, you may try running these commands to on Homestead to get php-redis working:

  1. vagrant ssh
  2. sudo apt-get update
  3. sudo apt install php-redis
  4. Go to app/config/database.php and change value from "predis" to "phpredis" for value "REDIS_CLIENT" (not always required)
  5. Go to app/config/app.php and rename Predis to PredisManager under Aliases
  6. sudo service php7.3-fpm restart (replace "7.3" with whatever version of PHP as required - you can check with command "sudo php -v")

That should be it! Worked for me.

laravel 6.4.0 is it ok?

@joelwmale
Copy link
Contributor

I had the same issue today when upgrading a 5.8.x app to 6.x. Followed the install guide at https://github.com/phpredis/phpredis/blob/develop/INSTALL.markdown as recommended by the docs, and verified redis was a loaded extension in php.

When deploying we received:

Please remove or rename the Redis facade alias in > your "app" configuration file in order to avoid collision with the PHP Redis extension.

This was fixed by running sudo apt install php-redis. Can this perhaps be added to the docs @driesvints?

@mfn
Copy link
Contributor

mfn commented Nov 5, 2019

I think the problem is that https://laravel.com/docs/6.x/upgrade#redis-default-client makes some assumptions on the readers knowledge:

The default Redis client has changed from predis to phpredis. In order to keep using predis, ensure the redis.client configuration option is set to predis in your config/database.php configuration file.

One has to know that phpredis is actually https://github.com/phpredis/phpredis which is installed via pecl install redis (i.e. it's not named phpredis here!) and the extension for packages is called php-redis (similar but yet another way for the same phpredis thing).

@joelwmale
Copy link
Contributor

I think the problem is that https://laravel.com/docs/6.x/upgrade#redis-default-client makes some assumptions on the readers knowledge:

The default Redis client has changed from predis to phpredis. In order to keep using predis, ensure the redis.client configuration option is set to predis in your config/database.php configuration file.

One has to know that phpredis is actually https://github.com/phpredis/phpredis which is installed via pecl install redis (i.e. it's not named phpredis here!) and the extension for packages is called php-redis (similar but yet another way for the same phpredis thing).

Yep you're 100% correct. My previous comment was not 100% accurate I apologize. The docs actually just say phpredis and when googled the first result is the github page for the pecl extension. Perhaps that could be cleared up in the docs as an outcome to this issue?

@driesvints
Copy link
Member

Feel free to send in a pr to the docs if you feel that something can be clarified.

@Arkanius
Copy link
Contributor

Arkanius commented Nov 11, 2019

Isn't this clear enough? laravel/docs#5388

@joelwmale
Copy link
Contributor

Isn't this clear enough? laravel/docs#5388

No that documentation is incorrect. As per this issue the correct php-redis is apt install php-redis not pecl install phpredis.

@driesvints
Copy link
Member

@joelwmale that'll only work on unix based systems though

@pheeque
Copy link

pheeque commented Dec 12, 2019

Set REDIS_CLIENT=predis in your .env file. Solved the problem I had with horizon for me anyway.

@darlanthiago
Copy link

Set REDIS_CLIENT=predis in .env file

@ahmed-bhs
Copy link

@LKaemmerling does there a way to force apt install php8.2-redis install the 5.3.7 version of phpredis instead of 6.0.0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests