search

geocoder-php / GeocoderLaravel

Geocoder service provider for Laravel
http://geocoder-php.org/GeocoderLaravel/
MIT License
704 stars 102 forks source link

Cache Duration should be defined in Seconds for Laravel 5.8+ #169

Closed bcwaretx closed 4 years ago

bcwaretx commented 4 years ago

General Information

GeocoderLaravel Version: 4.3 Laravel Version: 6.18.1 dev (Since 5.8) PHP Version: 7.2.5+ Operating System and Version: MacOS 10.11 MAMP Pro 5.5

Issue Description

Beginning with Laravel 5.8, cache TTL is defined in Seconds rather than Minutes. This was identified as a major breaking change: https://laravel.com/docs/5.8/upgrade#cache-ttl-in-seconds https://github.com/laravel/framework/pull/27276

Steps to Replicate

1) configure Redis caching per documentation 2) in Geocoder.php config, set cache.duration = 15 (minutes per documentation) 3) clear Redis cache (no keys present) 4) execute a geocode() call, verify Redis shows 1 key 5) execute same geocode(), verify Redis shows a cache hit 6) Wait 20 seconds, verify Redis shows 0 keys 7) execute same geocode(), verify Redis key is again added

Stack Trace

n/a

Since the Caching change is a major and documented breaking issue in Laravel, I am not sure if it would be preferred to only make a documentation update in the "Cache Duration" section of the config file, or update the library to convert provided parameter in Minutes into Seconds for the caching calls. Since the latter would potentially introduce a breaking change to applications that have already been modified for Laravel 5.8+, the documentation update may be the best route.

Thanks for a very helpful library

mikebronner commented 4 years ago

Hi @bcwaretx, thanks for reaching out! I'm a bit confused, though, as this package's cache configuration is already measured in seconds, as the documentation in the config file states. Can you clarify why you think it is in minutes?

smknstd commented 4 years ago

@mikebronner not sure, on master branch it's still written "minutes" in config file

Specify the cache duration in minutes

https://github.com/geocoder-php/GeocoderLaravel/blob/master/config/geocoder.php#L30

and also in readme:

The default cache duration provided by the config file is 999999999 minutes, essentially forever.

https://github.com/geocoder-php/GeocoderLaravel/blame/master/README.md#L102

mikebronner commented 4 years ago

Gotcha ... thanks for pointing that out! I'll get that fixed.