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

BUGFIX: Same bucket, different keyPrefixes, treat as "Two Buckets" mode #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

baschny
Copy link
Contributor

@baschny baschny commented Jul 4, 2023

We have a setup like this:

Neos:
  Flow:
    resource:
      storages:
        s3PersistentResourcesStorage:
          storage: 'Flownative\Aws\S3\S3Storage'
          storageOptions:
            bucket: 'our-bucket'
            keyPrefix: 'storage/'
      targets:
        s3PersistentResourcesTarget:
          subdivideHashPathSegment: true
          target: 'Flownative\Aws\S3\S3Target'
          targetOptions:
            bucket: 'our-bucket'
            keyPrefix: 'public/'
            baseUri: 'https://....cloudfront.net/public/'

This works great with Neos v7 and now we are upgrading to Neos v8, so we needed to upgrade this package from v2.4.2 to v2.6.2, which includes the "new feature" #49, which was later updated with this fix #63.

Now in our setup we are using just one bucket, but since the prefix is different, we want to make the publishing really "copy" the file over as it was before, so that we get a "pretti(er)" URL with the filename included. This is not working anymore: You end up with broken images in the backend, because its not copying the files at all.

This fix simply adds the check if also the prefix is also not the same, and then fallbacks to the "good old two buckets" mode as before.

@baschny
Copy link
Contributor Author

baschny commented Jul 4, 2023

@kdambekalns what do you think?

@baschny baschny changed the title Same bucket, different keyPrefixes, treat as "Two Buckets" mode BUGFIX: Same bucket, different keyPrefixes, treat as "Two Buckets" mode Jul 4, 2023
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest copying the file when different prefixes are used sound a) totally reasonable, b) like the way to go and c) a good idea. 😉 Otherwise using different prefix simply isn't possible.

What do you think, @robertlemke?

@kdambekalns kdambekalns requested a review from robertlemke July 5, 2023 16:53
Copy link
Member

@robertlemke robertlemke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! If you tested this for real, let's merge and release it.

@baschny
Copy link
Contributor Author

baschny commented Jul 6, 2023

I just tested our use case, which was broken after this FEATURE was introduced. I have not tested the other possible setup variations, but I expect that this change at least does not break anything that was not yet broken anyway.

@kdambekalns kdambekalns self-assigned this Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants