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

API Implement cache / move to silverstripe vendor #10

Closed tractorcow closed 7 years ago

tractorcow commented 7 years ago

See https://github.com/silverstripe/silverstripe-assets/issues/43 for parent story

Still need to write unit tests, but this can be peer reviewed in the mean time

sminnee commented 7 years ago

What's the best way to set up an integration test rig for this? Does it just need an S3 bucket?

tractorcow commented 7 years ago

I was thinking about using a mock for the s3 client, actually.

sminnee commented 7 years ago

Yeah then it wouldn't be an integration test, it'd be a component test.

With a vanilla S3 bucket / subfolder, would it be feasible to do an integration test of this with the substantial caveat that protected assets were publicly accessible?

tractorcow commented 7 years ago

In order to test this out:

Then flush, and that should be sufficient to get this installed and working.

When testing this PR it may fail to autoload psr-4 due to the updated namespace and psr-4 root. you will need to set a custom repository and install from composer that way.

{
    "require": {
        "silverstripe/s3": "dev-pulls/1.0/cache-me-riding-dirty"
    },
    "repositories": {
        "open-sausages": {
            "type": "vcs",
            "url": "git@github.com:open-sausages/silverstripe-s3.git"
        }
    }
}
sminnee commented 7 years ago

Edge-case issue: if you don't have write permission on the bucket, the error isn't helpful https://gist.github.com/sminnee/24f5831a0aa87c18b12d5918ea04aad6 - it would be better if it said "The webserver doesn't have permission to write the file to S3" or something in the response.

sminnee commented 7 years ago

OK — I've got this working but there's a bug. Draft assets are using the public asset URLs for their thumbnails, which don't yet exist and so throw "forbidden" errors.

tractorcow commented 7 years ago

Yes, something broken recently. Looking at this bug now.

tractorcow commented 7 years ago

I've traced this to a bug where exists() would use getMetadata(), and rely on potentially cached data which never actually calls the backend. We need a concept for agnostic-exists (will invoke a cache miss).

tractorcow commented 7 years ago

Updated with tests @sminnee

The public url bug is fixed.

tractorcow commented 7 years ago

Tests green. :P

sminnee commented 7 years ago

I've done a local test and it works for me now and the code looks pretty good.