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

Phalcon\Url::get and double slashes #14331

Closed
scrnjakovic opened this issue Aug 26, 2019 · 1 comment
Labels
Projects

Comments

@scrnjakovic
Copy link
Contributor

@scrnjakovic scrnjakovic commented Aug 26, 2019

The implementation of this method in v3.4 may return links like:
https://www.example.com//images/booya.jpg

I see that this has been reported and fixed for v4 in #13495

However, why don't we utilize our own helper and replace these 8 lines

cphalcon/phalcon/Url.zep

Lines 166 to 174 in e78ab44

if substr(baseUri, -1) == "/" && strlen(strUri) > 2 && strUri[0] == '/' && strUri[1] != '/' {
let uri = baseUri . substr(strUri, 1);
} else {
if baseUri == "/" && strlen(strUri) == 1 && strUri[0] == '/' {
let uri = baseUri;
} else {
let uri = baseUri . strUri;
}
}

with

let uri = \Phalcon\Helper\Str::reduceSlashes(baseUri . strUri);

furthermore, we can reduce the overhead of invoking an extra function and directly use

let uri = preg_replace("#(?<!:)//+#", "/", baseUri . strUri);

The only scenario in which this wouldn't work is if we passed an url with relative protocols like //netdna.bootstrapcdn.com/twitter-bootstrap/2.3.4/css/bootstrap-combined.min.css but in that case, that wouldn't be a local URL, therefore it does not pose a problem.

cc @niden @sergeyklay @Jurigag

@ruudboon ruudboon added this to To do in 4.0 Release via automation Aug 26, 2019
@scrnjakovic scrnjakovic changed the title Phalcon\Url\Get and double slashes Phalcon\Url::get and double slashes Aug 26, 2019
@ruudboon ruudboon moved this from To do to In progress in 4.0 Release Aug 26, 2019
@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Aug 26, 2019

Resolved

@niden niden closed this Aug 26, 2019
4.0 Release automation moved this from In progress to Done Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.0 Release
  
Done
3 participants
You can’t perform that action at this time.