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

feat(url_for): convert backward to forward slash #228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

curbengh
Copy link
Contributor

@curbengh curbengh commented Aug 15, 2020

to deal with Windows path, see https://github.com/hexojs/hexo/pull/4479/files#r470968350

url_for() deals with relative URLs, which should never have backward slash.

full_url_for() doesn't need to update because encodeURL() already have conversion in place (via URL API).

I'm assuming no one uses backward slash "%5C" in urls.

@curbengh
Copy link
Contributor Author

curbengh commented Aug 15, 2020

Alternatively, we could also convert in encodeURL() instead (used by both url_for() and full_url_for().


doesn't work quite well in encodeURL(), e.g. url_for('\\foo\\bar').

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.424% when pulling 7f97f79 on curbengh:url_for-backslash into b34456a on hexojs:master.

@SukkaW
Copy link
Member

SukkaW commented Aug 15, 2020

url_for is designed to transform the URL path. It should not be used to convert file system path into URL.

@curbengh
Copy link
Contributor Author

curbengh commented Aug 16, 2020

full_url_for() currently converts slash (via URL API) whereas url_for() percent-encode it, this PR makes them behave similarly.

const { format } = require('url');

const lorem = new URL('http://ipsum.com/foo\\bar/baz\\dolor')

console.log(format(lorem))
// http://ipsum.com/foo/bar/baz/dolor

@curbengh curbengh marked this pull request as ready for review August 17, 2020 10:02
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