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

Suggest predis instead of requiring it #381

Closed
wants to merge 1 commit into from

Conversation

ThomHurks
Copy link
Contributor

Just like laravel/framework which suggests predis (and people can choose to use phpredis instead), Horizon should suggest it so people using phpredis do not needlessly install a dependency.

Horizon relies on the redis config in config/database.php anyway, and trusts that the user correctly fills in that information. Requiring the user to install phpredis or predis herself is no different from what other parts of Laravel using Redis already do.

Just like laravel/framework which suggests predis (and people can choose to use phpredis instead), Horizon should suggest it so people using phpredis do not needlessly install a dependency. Horizon relies on the redis config in config/database.php anyway, and trusts that the user correctly fills in that information. Requiring the user to install phpredis or predis herself is no different from what other parts of Laravel using Redis already do.
@taylorotwell
Copy link
Member

Maybe on the next major version. I don't want to uninstall Predis from people's working applications on a point release.

@ThomHurks
Copy link
Contributor Author

@driesvints @taylorotwell Can we add this to version 3.0 maybe, since it was only just released?

@browner12
Copy link
Contributor

since it's released already, we can't make a change like this, even if it was just yesterday.

I would suggest PR'ing it to master.

@driesvints
Copy link
Member

Yes please pr to master but it'll be a while maybe before we make another major release.

@ThomHurks
Copy link
Contributor Author

@driesvints Created PR #531 to Master.

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.

4 participants