Closed jacekkarczmarczyk closed 2 years ago
Hello curious contributor ! Since it seems to be your first contribution, make sure that you've been:
Hello,
Nice catch, but I'm facing one potential problem with DateTimeImmutable object.
I'm not sure how atomic operations such as $item::expiresAfter()
should react if Immutable object are set 🤔
I think that I should convert immutable object to mutable object before passing them to the core.
After some code review, I've decided that Immutable DateTime will be automatically converted to Mutable DateTime for one simple reason: Very few backend support "plain DateTime" object storage. In fact for most of them I have to store the date as an ATOM formatted string .
So I cannot even persist the mutability status of the Datetime object along with other stored data.
Every solution is fine as long as it doesn't throw error when reading data saved with DateTimeImmutable expiration date :)
Indeed, however if I fix that error I will introduce an unintended side-effect that I'll need to fix too, so I need to fix both of them.
Can you test the v9 branch please ?
If it's ok for you I'll push a fix for v8.1 tomorrow too.
Thanks.
Thank you for the patch, I have some comments and problems:
composer require phpfastcache/phpfastcache:v9
it installed version 9.0, when I did composer require phpfastcache/phpfastcache:dev-v9
I got an error Could not find package phpfastcache/phpfastcache in a version matching "dev-v9" and a stability matching "dev".
(I have "minimum-stability": "dev",
in my composer.json
)\DateTime::createFromInterface
instead of \DateTime::createFromImmutable
?Update: I've managed to install it with composer require phpfastcache/phpfastcache:v9.x-dev
, I'm getting a different error though which now I'm investigating
Update2: new error was not related to phpfastcache, so looks good for me except these 2 small suggestions from the previous comment
I will fix them, thanks.
how about using \DateTime::createFromInterface instead of \DateTime::createFromImmutable
Finally, I will keep the code as is, since I will made the same fix on the v8.1 and the v8.1 is still php 7.3 compatible, however \DateTime::createFromInterface
has been added in PHP 8.
Since the 9.1.0 have been released recently, please except the 9.1.1 to be released by the end of the week, maybe the next one.
I've switched to DateTime as a workaround, when you release 9.1.1 I'll go back to DateTimeInterface, no hurry Thanks!
Configuration
Describe the bug
When updating cache item like this:
(which is valid as
setExpirationDate
allowsDateTimeInterface
)phpfastcache fails on reading data:
in
\Phpfastcache\Core\Pool\DriverBaseTrait::driverUnwrapEdate
Expected behavior No error when expiration date set to
DateTimeImmutable