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

Fix S3 filenames that include a hash #121

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

Conversation

cyreb7
Copy link

@cyreb7 cyreb7 commented Nov 14, 2024

Ran into this when we switched to using Zip Stream recently, S3 filenames with a hash # were being truncated. The Laravel Storage facade handles hashes without issue.

Possible breaking changes I can think of with this PR:

  1. Using strlen("s3://{$this->getBucket()}/"); means that if anyone has a malformed S3 bucket string, this would fail in unexpected ways
  2. If anyone was relying on the old behavior of parse_url(), this does change how S3 file paths are parsed

@jszobody
Copy link
Member

See #110

@cyreb7
Copy link
Author

cyreb7 commented Nov 14, 2024

Oh gosh, sorry, I didn't see that PR. I don't mind if you want to close this, but I'd argue that while AWS doesn't recommend hashes, they do support it. Plus at least two people have run into this in the wild and had the motivation to open a PR to fix it, so it's clearly a real-world issue.

Did you ever look into using the AWS parser? If you did, then we could at least bug AWS support instead of you 😆

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.

2 participants