Skip to content

Conversation

@lakuapik
Copy link
Contributor

@lakuapik lakuapik commented Apr 3, 2021

When we change the temporary_url config for S3, laravel does not include the port.

Example config

// ...
'endpoint' => env('AWS_ENDPOINT', 'https://example.com:8000'),
'temporary_url' => env('AWS_TEMPORARY_URL', 'http://temp.example.com:8888'),
//...

Storage::temporaryUrl() will return the port 8000 instead of 8888.

This PR resolve that issue.

$parsed = parse_url($url);

return $uri->withScheme($parsed['scheme'])->withHost($parsed['host']);
return $uri->withScheme($parsed['scheme'])->withHost($parsed['host'])->withPort($parsed['port']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to throw an error because $parsed['port'] won't exist in the array for a url without a port. (Like https://laravel.com)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah i see. It will give a warning <warning>PHP Notice: Undefined index: port in .... when the port is not specified.

My solution would be something like

return $uri->withScheme($parsed['scheme'])->withHost($parsed['host'])->withPort(@$parsed['port']);
// or
return $uri->withScheme($parsed['scheme'])->withHost($parsed['host'])->withPort($parsed['port'] ?? null);

Or what is the best solution in laravel way?

Copy link
Contributor

@BrandonSurowiec BrandonSurowiec Apr 3, 2021

Choose a reason for hiding this comment

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

Would do the second:

return $uri->withScheme($parsed['scheme'])->withHost($parsed['host'])->withPort($parsed['port'] ?? null);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GrahamCampbell GrahamCampbell changed the title Add missing port when replacing S3 temporary_url [8.x] Add missing port when replacing S3 temporary_url Apr 3, 2021
@taylorotwell taylorotwell merged commit cd8cbd8 into laravel:8.x Apr 5, 2021
@lakuapik lakuapik deleted the patch-filesystem-2 branch April 26, 2021 06:44
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