Kill predis support in Laravel 6.0

[GrahamCampbell]

predis appears to be no longer maintained, and already has existing compatibility issues with PHP 7.3+. Things are likely to only get worse as time progresses. Moreover, unmaintained software is bad from a security perspective, for the standard reasons. Should we kill support for it, and only support the phpredis 5.x extension in L6.0?

[GrahamCampbell]

// cc @driesvints

[crynobone]

I would prefer huge breaking changes such as this and Switch from SwiftMailer to Symfony Mailer way early before a major release going to go public so developers have time to plan out changes that might need to be done.

---

I been using predis for my production code since the beginning and yet to see major issue (storing 2-4GB of data), in fact we also been using predis-async for fews of our daemon services to listen to redis pubsub messages.

I'm not saying that we shouldn't deprecate/remove it but it would be better if Laravel 6.0 discourage people to use predis and maybe remove it for Laravel 7.0

[driesvints]

Let's keep the swiftmailer discussion on the PR okay?

I agree with you @crynobone. I think it might be best to start discouraging its use and get people to move to the C extension. It's unfortunate but until someone provides a maintained alternative we're only left with that option.

[crynobone]

My arguments are more toward the result of #383

I don't see why we can't agree to remove it in 7.0 if on the release of 6.0 we already make the community aware of the upcoming major breaks. Similar to the helpers removal.

[rimace]

@crynobone

I'm not saying that we shouldn't deprecate/remove it but it would be better if Laravel 6.0 discourage people to use predis

Isn't "deprecating a feature" the same as discouraging users to use it? From my understanding "deprecating" means "we support it, but please use alternatives because this one will be removed in a future version".

[crynobone]

Isn't "deprecating a feature" the same as discouraging users to use it?

In Laravel major release, "deprecation" something actually means direct removal.

• Lang::trans() removed without deprecation and you should use Lang::get()
• Lang::transChoice() removed without deprecation and you should use Lang::choice()
• Input facade was removed without deprecation

So when someone said let's kill of a feature, that is my response.

[driesvints]

@crynobone yeah the term is definitely misused sometimes. I think I'm guilty of that myself a few times in the past. It should definitely be understood that deprecation doesn't means removal but marking a feature to let people know it's going to be removed in a future version.

[michaeldyrynda]

I definitely think marking it as deprecated in 6.x and removing in 7.x is the way to go. Laravel works with the C extension already if I’m not mistaken, so flagging this in the upgrade guide will give folks a chance to get off predis and testing against C extension.

Doing it a week or two before the release, particular in the face of freshly adopting semver, doesn’t seem like the wisest approach.

[driesvints]

@crynobone I do think that in the changes you listed it wasn't warranted to deprecate first since those are only minor changes imo. They're easy to migrate from.

[crynobone]

@driesvints small removal for isn't much of an issue, but within weeks of upcoming major release is best to be avoided. if it was done early in 5.8 developers has lots of time to prepare but personally I only known the removal via Travis weekly cron feature.

[tillkruss]

Deprecation in 6.0 and removal in 6.1 seams reasonable.

Personally I wouldn’t mind removal in 6.0, since Predis and PhpRedis can be interchanged and if there is any discrepancy we can patch it, just like we did for PhpRedis to match the Predis API.

[driesvints]

@tillkruss removal would be in 7.0 at the earliest. Removing functionality in a minor release goes against SemVer.

[GrahamCampbell]

I'd be cool with that. We should defintely discurrage the use of the library, immediately tbh, so that if there is ever an important security problem discovered with the library that is not patched, people don't have to rapidly work out how to get the redis extension into production.

[driesvints]

Talked to Taylor about this. I'll send in a PR on Thursday to start deprecating the current functionality.

[crynobone]

A good start is to make sure https://github.com/laravel/horizon is properly tested with phpredis, from what I can see it tested only with REDIS_CLIENT=predis (default) at the moment.

[francislavoie]

I've been using predis + the phpiredis extension (note the i, different from phpredis) which predis can use as a backend for some extra speed. Generally worked fine for me. But I do also agree that it not being maintained for such a long time is unfortunate.

I think an issue of note with deprecating predis is that we don't have a migration path for people who want to use Redis but can't install PHP extensions for whatever reason. That might be a thin segment of users, but I thought it's worth mentioning. I haven't dug around in a couple years, but if possible, having a pure PHP alternative would be nice. Maybe a relatively active fork of predis exists?

[driesvints]

The problem is that more and more problems with Predis will start popping up over time. We're already seeing the first ones now with the new Redis 5 release.

Maybe a relatively active fork of predis exists?

I don't know of any unfortunately.

[francislavoie]

Maybe it's already a part of your plan, but maybe the predis driver could be broken out into an optional extension for those who don't specifically need Redis 5 support?

e.g. "If you can't install the phpredis extension and are using a version of Redis earlier than 5, then install laravel/predis-driver." Or something to that effect.

[michaeldyrynda]

I don't think Laravel has ever been afraid of moving things forward and I don't see a reason to start now. People that can't install extensions aren't the target for Laravel, I don't think. It's never suggested that you install Laravel on a shared web host for example, and Forge would likely be updated to ship with the C extension by default.

[tillkruss]

I wouldn’t bother with a Predis driver for Laravel as a package.

People will have plenty of time to migrate from 6.0 to 7.0 and the PHP 7.3 incompatibility as well as lack of Redis 5.0 support is incentive enough.

[driesvints]

@francislavoie everyone is free to build such a package/extension if they need it.

[driesvints]

I've sent in a PR for this here: laravel/framework#29688

[driesvints]

A good start is to make sure https://github.com/laravel/horizon is properly tested with phpredis, from what I can see it tested only with REDIS_CLIENT=predis (default) at the moment.

@crynobone I'll have a look at this in a sec 👍

[driesvints]

The PR was merged.

[francislavoie]

Github put a link to https://github.com/cheprasov/php-redis-client in my dashboard sidebar. This looks promising, supports PHP 7.3 and Redis 5. Could be investigated as an alternative to Predis for a pure-PHP option.

[driesvints]

@francislavoie thanks! We could look into that if Predis isn't getting maintained anymore. We recently contacted the maintainer of Predis and he might look for someone to take over.