-
Notifications
You must be signed in to change notification settings - Fork 24.4k
[6.x] Fix S3 endpoint url reference #5267
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
Conversation
Won't this break people's apps who are on older versions? How did this ever worked? |
AFAIK it shouldnt break anything, as it was not properly implemented before. Editing the I also just found out that it was commented on implementation of the 'url' parameter. dfd494f#commitcomment-29356206 |
Send to master branch. |
After this commit we get an |
And what about the |
@Brenneisen then just change it back? You own this file. |
@driesvints Just to inform you about possible problems that may result from this change. |
Just got hit by this one. The change from Like in all my other projects, I had this in my
But this results in: Storage::cloud()->url('test.jpg');
// Returns https://my-bucket.my-bucket.s3-eu-west-1.amazonaws.com/test.jpg Note the double bucket name in the url. Uploading files, reading files... it's all broken as well. Removing I know I can fix this myself, but this is not expected behavior for new projects. Also, this has worked like a charm for years for me, so I don't understand the change... |
Just change it back. You own this file. It's in your app. |
I also had problems with this last week when i synced my repository with this repository. In the end, i've reverted the change, as this does not work properly if you have a CNAME pointing to S3. From a semi quick check, Laravel internally uses
That's what i did in the end, but considering it's not only one person having problems with this change, it clearly seems something broke, and giving this kind of reply is not appropriate considering someone can clone the repository and have issues later on without knowing about this GitHub ticket and possibly "fix". |
@driesvints I changed it back immediately and just wanted to let you know that the change is buggy and will cause problems for new projects using this standard of laravel as project starting point. For me it is no problem, for new users it is, since this config is not compatible with the laravel framework. |
Sorry for the issues caused. |
Perfect, I just wanted to post this suggestion! @driesvints You're right, but Laravel should provide correct defaults, no? 🙂 |
@JacobHonore It's all good, there's ways to fix it manually so it was just a minor regression, at least for me. We're just raising awareness for the problem it created for some of us :) Renaming |
I like the suggestion of |
The reference to the changing the endpoint URL in s3 driver is named 'endpoint', not 'url'.
Also documented in the Flysystem docs:
https://flysystem.thephpleague.com/v1/docs/adapter/aws-s3-v2/#compatible-storage-protocols