search

mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.24k stars 214 forks source link

Implement PHPREDIS instead of PREDIS. #344

Closed wilbur-yu closed 4 years ago

wilbur-yu commented 4 years ago

predis was last updated in 2017, and laravel has also recommended phpredis, I personally think it should be replaced by phpredis

mikebronner commented 4 years ago

@wilbur-oo Thanks for the recommendation! :) I'll work on getting that updated. :)

mikebronner commented 4 years ago

The concern I have is with the configuration changes needed to make Phrpredis work with Laravel, while PREDIS works out of the box with no changes. That being said, PREDIS is struggling from a maintenance point of view. On the other hand, PHPREDIS requires the PECL extension to be installed, which may or may not be easy to do. Further, it's not installed in Homestead or on forge servers by default. For that reason I'm holding off for now until this becomes easier to manage in the future.

wilbur-yu commented 4 years ago

Yes, I agree with your concerns. But for me personally, the laravel-model-caching package is to improve production performance, so in order to achieve high performance, installing an extension should not be a big problem. Of course, this is just my personal opinion. I respect your ideas. After all, the change of an extension package ’s dependencies will affect too many users. Or I think it is possible to adapt two methods, that is, the willingness to use predis or phpredis is up to the user to decide.

mikebronner commented 4 years ago

Good points! Is there something in the package that is preventing you from using PHPREDIS? Since it's all on the configuration end, and this package doesn't configure the REDIS provider for you, you should be able to.

Would love to hear the steps you need to take to get PHPREDIS to work with the package, and if it throws any errors.

wolfpack4417 commented 4 years ago

I'm also having an issue getting PHP Redis working with this package. I decided on that instead of predis because of the warning in this section of the docs: https://laravel.com/docs/7.x/redis#introduction

I had used Predis before but if support is going to be removed from Laravel potentially I didn't want to waste time using it. Is that the only option for this library though? If so I might go back and use Predis until this package works with PHP Redis as well.

mikebronner commented 4 years ago

@wolfpack4417 Could you provide the stack trace of the errors you are getting? I'm thinking of removing the dependency on PREDIS all-to-gether, and not replacing it with PHPREDIS in this package, to leave it up to the user to implement one or the other of their choosing.

wolfpack4417 commented 4 years ago

Sorry I think this was on my end although I haven't figured it out yet. I think it has something to do with PHP Redis not being enabled on my docker container. I'm getting '1' when I run extension_loaded('redis') in the browser but not when I run php artisan modelCache:clear. I'll post back if I discover it wasn't an issue with my PHP Redis installation.

mikebronner commented 4 years ago

Thanks, that would be great! Ideally this package should be decoupled from your choice of caching driver. :)

mikebronner commented 4 years ago

Sorry for the delay on this. Please try release 0.10.0 and let me know how it works.

mikebronner commented 4 years ago

This is now easy, as Laravel Forge servers ship with PHPRedis already installed.