- Notifications
You must be signed in to change notification settings - Fork11.4k
[11.x] check not empty before concat url#55810
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:11.x
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Likely a breaking change: <?phpfunctionoriginalImplementation($url,$path){returnrtrim($url,'/') .'/' .ltrim($path,'/');}functionproposedImplementation($url,$path){if (empty($url)) {returntrim($path,'/'); }if (empty($path)) {returntrim($url,'/'); }returnrtrim($url,'/') .'/' .ltrim($path,'/');}$urls = ['','https://url.with.slash/','https://url.without.slash'];$paths = ['','/','path','path/','/path','/path/'];foreach ($urlsas$url) {foreach ($pathsas$path) {echooriginalImplementation($url,$path),PHP_EOL; }}echostr_repeat('-',80),PHP_EOL;foreach ($urlsas$url) {foreach ($pathsas$path) {echoproposedImplementation($url,$path),PHP_EOL; }} Output: $php test.php///path/path//path/path/https://url.with.slash/https://url.with.slash/https://url.with.slash/pathhttps://url.with.slash/path/https://url.with.slash/pathhttps://url.with.slash/path/https://url.without.slash/https://url.without.slash/https://url.without.slash/pathhttps://url.without.slash/path/https://url.without.slash/pathhttps://url.without.slash/path/--------------------------------------------------------------------------------pathpathpathpathhttps://url.with.slashhttps://url.with.slash/https://url.with.slash/pathhttps://url.with.slash/path/https://url.with.slash/pathhttps://url.with.slash/path/https://url.without.slashhttps://url.without.slash/https://url.without.slash/pathhttps://url.without.slash/path/https://url.without.slash/pathhttps://url.without.slash/path/ |
MrTuffaha commentedMay 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@rodrigopedra thanks for your observation, i have since updated the code so that if one parameter is empty it just returns the other parameter. There is still change in the result when the $url parameter is empty it doesnt prefix the $path with a "/" by default which is the fix i am proposing Here is the result from my new implementation: |
rodrigopedra commentedMay 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Well, it is still a breaking change, as other people might be relying on this behavior. I saw on the linked package that the prefix is user configurable, can't you just remove the starting slash on the prefix? I tested the linked package on a sample project, with one of the sample AZURE_STORAGE_LOCAL_ADDRESS=localAZURE_STORAGE_NAME=devstoreaccount1AZURE_STORAGE_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==AZURE_STORAGE_CONTAINER=container And added this key for the prefix: AZURE_STORAGE_PREFIX=foo Running this command: Artisan::command('app:test',function () {$this->comment(Storage::disk('azure')->url('bar.txt'));}); I got this output: $php artisan app:testhttps://devstoreaccount1.blob.core.windows.net/container/foo/bar.txt Can you provide the You can provide only which |
Maybe you can ask the package maintainer to wrap the prefix with Right here: |
| @@ -847,6 +847,10 @@ public function temporaryUploadUrl($path, $expiration, array $options = []) | |||
| */ | |||
| protected function concatPathToUrl($url, $path) | |||
| { | |||
| if (empty($url) || empty($path)) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Empty is the wrong function here. It does not check if the variable us the empty string, it checks if it looks like an enmpty-ish value, inc if it's 0. One should compare against'' instead.
When using packages that adds a default prefix in filesystem config (examplesteffjenl/laravel-azure-blob-storage).
The FilesystemAdapter adds a "/" before the path as it only checks if prefix is set not also if its not empty, this may result in a url with double slashes "//".
this pull checks if prefix is not empty before combining it with path