-
Notifications
You must be signed in to change notification settings - Fork 24
Bugfix: if config.absRefPrefix is used, fullUrl returns wrong absolute uri #353
base: master
Are you sure you want to change the base?
Conversation
…e uri. Example: config.absRefPrefix = /test/ results in http://example.com/test/test/page instead of http://example.com/test/page
// specified domain | ||
$url = 'http://' . $domain . '/' . $url; | ||
if(!preg_match('/^https?:\/\//i', $domain)) { | ||
$domain = 'http://' . $domain; |
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.
What about https support? Regex expression allows https, but $domain =
forgets that.
Btw., is this an old (separate) issue, which always existed (eventually add a new/separate github issue?)?
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.
Well, we have to make some assumptions. If the passed domain has a scheme, everything is ok, so no scheme is prepended. If there is no scheme - so the regex doesn't match - I think we can't reliably guess the correct scheme at this code line (because in fact, the domain could be any domain, which could have https support or not), so the best is to stick with http ;-)
Instead of the regex a parse_url call could be used to check, if there is a scheme. Or we could make the regex more general ('/[a-z]+:\/\//i'
).
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.
If the user of this method really wants to specify the scheme, it could be done with the $domain
parameter. Maybe it's more clear to rename the parameter from $domain
to something like $baseUrlOverride
PR accidently got closed when dropping the old Still needs to be reviewed. Reopened it again. |
Fixes #354