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

[6.x] Fix S3 endpoint url reference #5267

Merged
merged 1 commit into from Mar 24, 2020
Merged

Conversation

JacobHonore
Copy link
Contributor

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

@GrahamCampbell GrahamCampbell changed the title Fix s3 endpoint url reference [6.x] Fix s3 endpoint url reference Mar 24, 2020
@GrahamCampbell GrahamCampbell changed the title [6.x] Fix s3 endpoint url reference [6.x] Fix S3 endpoint url reference Mar 24, 2020
@driesvints
Copy link
Member

Won't this break people's apps who are on older versions? How did this ever worked?

@JacobHonore
Copy link
Contributor Author

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 url parameter did not have any effect.
The issue is also referred to in https://github.com/thephpleague/flysystem-aws-s3-v3/issues/182 - but it is really an Laravel issue as it using the wrong parameter name.
The AWS-SDK-PHP v3 is also referring to the endpoint parameter name: https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/guide_configuration.html#endpoint

I also just found out that it was commented on implementation of the 'url' parameter. dfd494f#commitcomment-29356206

@taylorotwell taylorotwell merged commit b7b6e35 into laravel:6.x Mar 24, 2020
@taylorotwell
Copy link
Member

Send to master branch.

@Brenneisen
Copy link

Brenneisen commented Mar 30, 2020

After this commit we get an InvalidArgumentException in Aws\ClientResolver: Endpoints must be full URIs and include a scheme and host.

@Brenneisen
Copy link

And what about the getAwsUrl method in Illuminate\Filesystem\FilesystemAdapter?

@driesvints
Copy link
Member

@Brenneisen then just change it back? You own this file.

@Brenneisen
Copy link

@driesvints Just to inform you about possible problems that may result from this change.

@klaasgeldof
Copy link

Just got hit by this one. The change from url to endpoint in the config file is wrong imo.

Like in all my other projects, I had this in my .env file:

AWS_ACCESS_KEY_ID=...
AWS_SECRET_ACCESS_KEY=...
AWS_DEFAULT_REGION=eu-west-1
AWS_BUCKET=my-bucket
AWS_URL=https://my-bucket.s3-eu-west-1.amazonaws.com

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 AWS_URL from the .env file does fix it.

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...

@driesvints
Copy link
Member

Just change it back. You own this file. It's in your app.

@brunogaspar
Copy link
Contributor

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 url within the Filesystem implementation, that might be where the problem lies, but i didn't had enough time to perform a deeper investigation so i can be totally wrong.

Just change it back. You own this file. It's in your app.

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".

@Brenneisen
Copy link

Brenneisen commented Apr 9, 2020

@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.

@JacobHonore
Copy link
Contributor Author

Sorry for the issues caused.
The problem is that I thought 'url' was meant to change the endpoint URL. But this is changing the URL used for links to files. I found no documentation for this anywhere.
I will put in a new PR that introduces a new ENV variable, AWS_ENDPOINT and point that to 'endpoint', and then point AWS_URL back to 'url'.
I think the ENV variable naming is wrong though, it should be called S3_URL and S3_ENDPOINT, but I dont know how that should be fixed.

@klaasgeldof
Copy link

Sorry for the issues caused.
The problem is that I thought 'url' was meant to change the endpoint URL. But this is changing the URL used for links to files. I found no documentation for this anywhere.
I will put in a new PR that introduces a new ENV variable, AWS_ENDPOINT and point that to 'endpoint', and then point AWS_URL back to 'url'.
I think the ENV variable naming is wrong though, it should be called S3_URL and S3_ENDPOINT, but I dont know how that should be fixed.

Perfect, I just wanted to post this suggestion!

@driesvints You're right, but Laravel should provide correct defaults, no? 🙂

@brunogaspar
Copy link
Contributor

@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 env keys, might be best to do on the master branch i assume, so i guess you can propose the change in a pull request?

@driesvints
Copy link
Member

I like the suggestion of AWS_ENDPOINT as well. Just sent in a PR to offer both options so people can choose: #5276

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants