search

silverstripe / silverstripe-s3

Silverstripe module to store assets in S3 rather than on the local filesystem (SS4/SS5 only)
BSD 3-Clause "New" or "Revised" License
20 stars 25 forks source link

Replace custom caching with Flysystem caching adapter #20

Closed filiplikavcan closed 5 years ago

filiplikavcan commented 5 years ago

This PR removes any custom caching adapters (metadata and content) and just setup https://github.com/thephpleague/flysystem-cached-adapter for metadata caching. If somebody needs a content cache it can be done as a custom cache adapter which would just decorate cache adapter setup in this PR (which decorates AWS S3 adapter).

falnyr commented 5 years ago

@halkyon @madmatt

How is the PR looking?

obj63mc commented 5 years ago

Testing this pull request in general - it looks like this will fix issue #19 So far no issues and the CMS is working as expected.

madmatt commented 5 years ago

Hey @filiplikavcan, sorry for the delay reviewing this - my GitHub notifications don't get a huge amount of attention I'm sorry.

This changes quite a bit of how the module works (which I'm generally in favour of), but can you explain why you've removed the Travis config, changelog and other unit tests? I see there's a lot less code, but I would hope there's still something we can test there?

@obj63mc: The reason for the older caching is that S3 API access is pretty slow (especially for local development environments, but even within AWS it can occasionally be seconds before API responses are received, and we were previously doing a couple of API calls for every asset shown in /admin/assets. Have you found this to be much faster using this PR?

obj63mc commented 5 years ago

@madmatt in terms of speed I never really saw any issues. The biggest thing I saw was if I uploaded an image in the Files section of the CMS it was there in the protected version of the folder as expected. If I then published the image, it would show a broken preview and thumbnail trying to show some version still in the protected folder. Then if you try to use that image on the site some where if you are ever in 'stage' mode it is always showing a broken image. If I unpublish the image in files, it will then show properly in 'stage' but is then broken in 'live' mode. Lastly, if I then go back to the page then publish the page thus publishing the image, it is then broken in 'stage' mode again...

Simply using the code as submitted with this push, I get no issues at all the CMS 'just works'.

This issues were present today when tested from a plain install of SS4.2.2 and s3 module 0.3.0 with composer.

I then tried installing the s3 module from this fork and all issues were resolved.

chillu commented 5 years ago

I've given @obj63mc write access to the repo now, since he's running this module on various production sites, and has shown an interest in maintaining this (checked with @madmatt and @robbieaverill beforehand). @obj63mc You're welcome to review and merge some PRs :)

madmatt commented 5 years ago

Thanks for making these changes @filiplikavcan, and thanks for your comments around testing this @obj63mc! I'm happy to merge this in now, but I'd really like to get some unit tests back in place soon.