ENH Allow base URL to not have trailing slash #1438
Conversation
GuySartorelli
force-pushed
the
pulls/2/base-url-no-trailing-slash
branch
3 times, most recently
from
bfbfe27 to
a67dcd3
Compare
client/src/lib/urls.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| export const joinLinks = (...links) => { | |||
There was a problem hiding this comment.
The name is strange as it's not joining links, but parts of a link/URL. Perhaps joinUrlParts? or joinLinkParts? Or make it actually a universal helper in the style of implode that takes parts and the glue?
Could be something like https://locutus.io/php/strings/implode/ (simplified) or just used as a dependency.
There was a problem hiding this comment.
Yeah I didn't spend much time thinking about the name tbh, it was just meant to mimic the most common use-case for Controller::join_links() in php-land. But it doesn't handle all of the cases that method does so you're right that it can't truly join all links as the name might imply.
I'll update the relevant PRs to change this to joinUrlPaths as that's really what it's doing - it is only concerned with the path portion of a url.
While I could make it a universal helper, the intention is to explicitly only include one of the character in-between the two parts (to avoid ending up with some///path as an extreme example). The implode implementation wouldn't do that, and making this more generic I think would necessarily involve adding additional complexity to make that an optional behaviour - and frankly I've spent too long on this already 😅
There was a problem hiding this comment.
Fair enough, changing to joinUrlParts = (...urlParts) => { will suffice.
There was a problem hiding this comment.
Done, except I used paths instead of parts because I think it's more correct (it doesn't distinguish between, say, query strings or schemes etc so you shouldn't use it to join the different parts of a URL together), and because I already made that change before your last comment :p
GuySartorelli
force-pushed
the
pulls/2/base-url-no-trailing-slash
branch
3 times, most recently
from
1d3eced to
9efa52a
Compare
client/src/lib/Router.js
Outdated
| // Remove base url from absolute path, save for trailing `/` which Page.js requires | ||
| return absolutePath.substring(absoluteBase.length - 1); | ||
| // Remove base url from absolute path, save for leading `/` which Page.js requires | ||
| const regex = new RegExp(`^${absoluteBase}/?`); |
There was a problem hiding this comment.
Should regex escape asbsoluteBase e.g. . should become \.
Though why not just use the substring above? then ltrim any '/' and then prefix '/' back in
There was a problem hiding this comment.
rtrim and suffix - it's meant to be on the end. But regardless, there's no simple rtrim function in javascript.
And even if there was, this doesn't just "remove and add a slash", it's removing the absoluteBase from the absolutePath. e.g. if absoluteBase is https://www.example.com/ and absolutePath is https://www.example.com/some-path/, the result of this operation is /some-path/.
You're correct that the variable needs to be escaped, though. I'll use lodash for that.
client/src/legacy/LeftAndMain.js
Outdated
| @@ -55,7 +55,7 @@ window.ss.tabStateUrl = function() { | |||
| return window.location.href | |||
| .replace(/\?.*/, '') | |||
| .replace(/#.*/, '') | |||
| .replace($('base').attr('href'), ''); | |||
| .replace(new RegExp(`^${$('base').attr('href')}/?`), ''); | |||
There was a problem hiding this comment.
Should regex escape the base e.g. . should become \.
Or just ltrim('/') the base and then prefix it with '/' (better imo)
There was a problem hiding this comment.
There's no ltrim or rtrim in javascript. I don't see any downsides to using regex for this. I'll escape the string appropriately.
client/src/lib/urls.js
Outdated
| @@ -0,0 +1,8 @@ | |||
| export const joinUrlPaths = (...links) => { | |||
GuySartorelli
force-pushed
the
pulls/2/base-url-no-trailing-slash
branch
from
9efa52a to
90ba5f1
Compare
| @@ -289,7 +290,7 @@ ss.editorWrappers.tinyMCE = (function() { | |||
| if(cb) href = eval(cb + "(href, node, true);"); | |||
|
|
|||
| // Turn into relative, if set in TinyMCE config | |||
| if(cu && href.match(new RegExp('^' + tinyMCE.settings['document_base_url'] + '(.*)$'))) { | |||
| if(cu && href.match(new RegExp('^' + escapeRegExp(tinyMCE.settings['document_base_url']) + '(.*)$'))) { | |||
There was a problem hiding this comment.
This isn't directly related to the purpose of this PR, but I'm adding escapeRegExp in this PR so it makes sense to apply it in the one other place I found that part of a URL is used as a variable in new RegExp().
|
That unit test will fail until the framework PR is merged. Refer to the installer build in the main issue. |
silverstripe/silverstripe-framework#10538 allows us to have a base URL which doesn't have a trailing slash. This PR updates the admin javascript to stop explicitly expecting one.
Parent issue