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

Compatibility with PSR-6/PSR-16 #70

Open
wants to merge 4 commits into
base: 2.4.x
Choose a base branch
from
Open

Conversation

codealfa
Copy link

Q A
BC Break yes
New Feature yes

Description

As requested in #7, I have implemented compatibility with PSR-6 and PSR-16 in the filesystem adapter. Since the filesystem is the storage that is available on every server and hence the defacto storage, then at the very least, I think this adapter should support these standards.

I have implemented static TTL capabilities by prepending a line containing a token with the expiration time when a file is saved to the cache. I have added another method, expired to the LocalFilesystemInteraction class that reads the first line and verifies that the file is still valid. If the cache is expired, the file is deleted and the method returns true, otherwise false. This is done to avoid reading the entire file into memory only to check its status as in the case of clearExpired or getTags. If the file will be read as in getItem or getItems, then the expiration is checked after the file is read and appropriate action is taken to avoid reading the file twice. If the file is expired in either case, it is promptly deleted.

Additionally, I have added the . character to the default KEY_PATTERN in FilesystemOptions to comply with the PSR-6 and PSR-16 requirements.

I have also implemented the FilesystemIntegrationTest for PSR-6 and have removed all test skips relating to TTL and valid keys for both PSR-6 and PSR-16. The only test skipped now is testBasicUsageWithLongKey, for which I did not understand the rationale. It is testing for a key length of 300 characters, and the skip message states, 'Filesystem adapter supports only 64 characters for a cache key'. However, the max key length in the Filesystem adapter capabilities is nearly 255 and the PSR requirement is "up to 64 characters." In my opinion, the test would be more useful if testing for support of a minimum of 64 characters, and then we would not have to skip anything tests.

Two more tests were added to FilesystemTest to verify files were deleted when expired, and that program flow wasn't interrupted if expired cache wasn't deleted immediately, but still reported as invalid.

Signed-off-by: codealfa <samuel@jch-optimize.net>
Signed-off-by: codealfa <samuel@jch-optimize.net>
Signed-off-by: codealfa <samuel@jch-optimize.net>
Signed-off-by: codealfa <samuel@jch-optimize.net>
@boesing
Copy link
Member

boesing commented Jul 25, 2023

Thanks! I will have a look on this when I find some time, might be this weekend.
I'll add some labels and see what I can do. I will also add PSR-6 & PSR-16 capabilities to laminas-cache v3 which can be released once I finished the work on laminas-serializer v3.

Afterwards, I start to implement v3 compat in all adapters and thus, could merge this PR as well (which might need some adjustments regarding the new major versions of PSR-16 an PSR-6.

So there will be a bunch of new majors in the next weeks, pretty sure I can add this to the next major as well.


Some quick thoughts:

  • yes, TTL in a dedicated line could work, but I probably would prefer a dedicated file to persist it (afaik its done with separate files for tags as well)
  • the biggest issue I have with TTL capability in filesystem is, that the filesystem does not autoclean which means that once a cache item is not requested anymore, it permanently consumes storage. Thats also the reason why I decided to not implement it by myself, but I see why one would like to use it and thus, I will consider this even tho my personal feeling is to not support it as

I will double-check the reason on why the long key test skip message states that a maximum of 64 characters is supported. That seems to be incorrect and might've been a copy & paste issue.

@codealfa
Copy link
Author

No worries. Happy for the chance to contribute. I was looking for a PSR-compliant caching system and found Laminas. I have been using it for about two years without the PSR decorators anyway to take advantage of some of the native features and fell in love with it, but now I've decided to implement PSR conformance in my project but was disappointed that the filesystem adapter didn't support it. I decided to share my integration, but if you decide on another implementation, that's ok too, as long as we have full PSR compatibility sooner than later.

TTL in a dedicated line could work, but I probably would prefer a dedicated file to persist it

You did mention in #7 your concern for much I/O with this implementation, with which I'm in total agreement. That's why I implemented it this way.

the biggest issue I have with TTL capability in filesystem is, that the filesystem does not autoclean which means that once a cache item is not requested anymore, it permanently consumes storage.

The only way I know how to address the storage of expired resources in the filesystem is by using the clearExpired function or something similar. Based on how I implemented it, the clearExpired function will still delete any expired cache. The difference now is that we're no longer relying on modified time to check the validity of the cache. We're calling the newly added expired method in LocalFilesystemInteraction, which reads the first line to determine if the cache has expired. If it is, the file is deleted, and the function returns true.

What I wasn't sure of was how you'd want to go about the deletion of expired files when they are requested. I think this should have minimum impact on program flow in the event the deletion wasn't successful. Anyway, whenever you've got a chance to review, you can decide how to move forward, and I'll be happy to assist in any way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants