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

Cleanup capabilities #8

Closed
weierophinney opened this issue Dec 31, 2019 · 3 comments · Fixed by #308
Closed

Cleanup capabilities #8

weierophinney opened this issue Dec 31, 2019 · 3 comments · Fixed by #308
Labels
Enhancement Question Further information is requested

Comments

@weierophinney
Copy link
Member

Some of the current defined capabilities are very hard to understand/useless and I would like to define some of them.

minTtl
This defines the minimum supported TTL which is clear but only a handful people knows that this also defines the general TTL support.

  • 0 means TTL is supported

  • 0 means TTL is not supported and all other TTL related capabilities can be ignored

There is no adapter having another value than 0 or 1 so this can be renamed to ttlSupported with bool.

-> minTtl needs to be still available for BC reasons until the next major but can be marked deprecated.

staticTtl
This defines how the expiration time will be calculated which is not a meaningful description.

  • true means the expiration time will be calculated on read using the last-modification-time of the item, the current time and the current TTL option
  • false means the expiration time will be calculated using the current TTL and the current time. It will be stored together with the item

Thoughts for better names ?

expiredRead
There was the idea that it is possible to read an expired item by setting changing the TTL (like to 0 = infinitive) but this only works with staticTtl=true to make it possible using an expired item in cases regeneration breaks (like on failed DB connection).

This is a useless capability because changing the TTL to something else automatically changes the expiration time of all stored items if staticTtl=true which means in case for infinity the item is no longer expired. -> Bam the item is no longer expired but this capability is for reading expired items and the nature of staticTtl already defines this behavior.

-> I would like to deprecate this capability and remove it in the next major version.

@kynx @Maks3w @Ocramius @ezimuel Thoughts ?


Originally posted by @marc-mabe at zendframework/zend-cache#81

@weierophinney weierophinney added the Question Further information is requested label Dec 31, 2019
@weierophinney
Copy link
Member Author

staticTtl

Perhaps ttlOnRead?

While we're there, the format for supportedDatatypes is pretty confusing:

'object' => true, // OK, we can store objects
'object' => 'object', // OK, as above
'integer' => true, // OK, we can store integers
'integer' => 'string', // they're stored as strings (redis)
'integer' => 'integer', // huh? stored as string (memcache), but you wouldn't know it

Maybe the last is a bug, or maybe I'm missing something. I can see some value in being able say "yes, the adapter will store it, but won't return the same type", but true / false / returned type would be clearer.


Originally posted by @kynx at zendframework/zend-cache#81 (comment)

@weierophinney
Copy link
Member Author

@kynx The idea behind this is as follows:

  • true: Variable of this type will be stores as is and the following will be true:
$value = 100;
$storage->setItem('key', $value);
var_dump($value == $storage->getItem('key')); // true
var_dump($value === $storage->getItem('key')); // true
  • {type}: Variables of this type will be serialized and the following will be true:
$value = new stdClass;
$storage->setItem('key', $value);
var_dump($value == $storage->getItem('key')); // true
var_dump($value === $storage->getItem('key')); // false
  • false: Variable of this type can't be stored

In case for integer it should be true / false only because an integer will be same in strict mode if it's the same in non strict mode.


Originally posted by @marc-mabe at zendframework/zend-cache#81 (comment)

@boesing
Copy link
Member

boesing commented Aug 26, 2023

Hm, I will throw my two cents in here since I do use the cache component quite frequently in almost every project I am working on.

I've never used the capabilities method and thus never had a need for this.
I also do not understand the use-case of this.

Regarding the supportedDataTypes:
My expectation would be: Whatever I persist to the storage is returned in the same type.
If the adapter does not support specific types, it should either throw an exception in case there is no way to do that or it should internally handle the conversion.

Do we have any thoughts on this?
I think it is quite common to pass a serializer to the adapter anyways, i.e. IgBinary, PhpSerialize or whatever else is preserving the native type.

We (in our companies projects) do use Redis/RedisCluster internal IgBinary serializer by passing lib_options with:

['lib_options' => [RedisCluster::OPT_SERIALIZER => RedisCluster::SERIALIZER_IGBINARY]];

It does in fact require the igbinary extension and redis extension with enabled igbinary support.

I guess the supported data types can be removed and instead require components to actually handle conversion/throw exceptions.


TL;DR: I have never had the need to use the supported types since these are heavily depending on the serializer being used. The redis adapter has some special handling to provide serializer dependent types even tho it might be incorrect (as it is just assuming that types are being preserved.

boesing added a commit to boesing/laminas-cache that referenced this issue Jun 13, 2024
This also removes `capability` event and the `marker` object. Capabilities are meant to be read-only and thus won't change after retrieval via `StorageInterface#getCapabilties`.
By having the event deleted, there is no need to have the adapter injected, though we also removed `Capabilities#getAdapter` and the `StorageInterface` constructor argument.

By refactoring the `Serializer` plugin, which already replaced the initial instance of `Capabilities`, we were able to drop the `base capabilities` feature as well.

Final result is that we now provide all capabilities as public `read-only` properties which reflect their defaults which were previously handled via internal `$default` magic in `Capabilties#getCapability`.

Along all the refactoring, some capabilities have changed or were removed while respecting ideas of laminas#8 which was around since 2016:
- `staticTtl` got removed without a replacement. It outlined if the cache backend is handling cache expiry or if the implementation is taking care of it. Though it might be interesting for picking a specific backend, that is rather not of interest at runtime for upstream projects.
- `lockOnExpire` got removed without a replacement. Was only used in zend-server adapter which is already abandoned since 2022
- `minTtl` got renamed to `ttlSupported` and its type changed from `int` to `bool`
- `maxTtl` got removed without a replacement. Every cache backend which is supported by laminas right now does allow TTLs being an infinite amount of seconds (where maximum `int` range is the limit, depending on the CPU architecture). There was a backend `XCache` where a limit existed but that backend was abandoned in 2021.
- `useRequestTime` was renamed to `usesRequestTime`
- `namespaceSeparator` got removed without a replacement. It was reflecting the option value and this the storage options can be used instead.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
boesing added a commit to boesing/laminas-cache that referenced this issue Jun 13, 2024
This also removes `capability` event and the `marker` object. Capabilities are meant to be read-only and thus won't change after retrieval via `StorageInterface#getCapabilties`.
By having the event deleted, there is no need to have the adapter injected, though we also removed `Capabilities#getAdapter` and the `StorageInterface` constructor argument.

By refactoring the `Serializer` plugin, which already replaced the initial instance of `Capabilities`, we were able to drop the `base capabilities` feature as well.

Final result is that we now provide all capabilities as public `read-only` properties which reflect their defaults which were previously handled via internal `$default` magic in `Capabilties#getCapability`.

Along all the refactoring, some capabilities have changed or were removed while respecting ideas of laminas#8 which was around since 2016:
- `staticTtl` got removed without a replacement. It outlined if the cache backend is handling cache expiry or if the implementation is taking care of it. Though it might be interesting for picking a specific backend, that is rather not of interest at runtime for upstream projects.
- `lockOnExpire` got removed without a replacement. Was only used in zend-server adapter which is already abandoned since 2022
- `minTtl` got renamed to `ttlSupported` and its type changed from `int` to `bool`
- `maxTtl` got removed without a replacement. Every cache backend which is supported by laminas right now does allow TTLs being an infinite amount of seconds (where maximum `int` range is the limit, depending on the CPU architecture). There was a backend `XCache` where a limit existed but that backend was abandoned in 2021.
- `useRequestTime` was renamed to `usesRequestTime`
- `namespaceSeparator` got removed without a replacement. It was reflecting the option value and this the storage options can be used instead.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
boesing added a commit to boesing/laminas-cache that referenced this issue Jun 13, 2024
This also removes `capability` event and the `marker` object. Capabilities are meant to be read-only and thus won't change after retrieval via `StorageInterface#getCapabilties`.
By having the event deleted, there is no need to have the adapter injected, though we also removed `Capabilities#getAdapter` and the `StorageInterface` constructor argument.

By refactoring the `Serializer` plugin, which already replaced the initial instance of `Capabilities`, we were able to drop the `base capabilities` feature as well.

Final result is that we now provide all capabilities as public `read-only` properties which reflect their defaults which were previously handled via internal `$default` magic in `Capabilties#getCapability`.

Along all the refactoring, some capabilities have changed or were removed while respecting ideas of laminas#8 which was around since 2016:
- `staticTtl` got removed without a replacement. It outlined if the cache backend is handling cache expiry or if the implementation is taking care of it. Though it might be interesting for picking a specific backend, that is rather not of interest at runtime for upstream projects.
- `lockOnExpire` got removed without a replacement. Was only used in zend-server adapter which is already abandoned since 2022
- `minTtl` got renamed to `ttlSupported` and its type changed from `int` to `bool`
- `maxTtl` got removed without a replacement. Every cache backend which is supported by laminas right now does allow TTLs being an infinite amount of seconds (where maximum `int` range is the limit, depending on the CPU architecture). There was a backend `XCache` where a limit existed but that backend was abandoned in 2021.
- `useRequestTime` was renamed to `usesRequestTime`
- `namespaceSeparator` got removed without a replacement. It was reflecting the option value and this the storage options can be used instead.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants