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

fix: encode permalink by default #3708

Merged
merged 7 commits into from Sep 20, 2019
Merged

fix: encode permalink by default #3708

merged 7 commits into from Sep 20, 2019

Conversation

@curbengh
Copy link
Contributor

@curbengh curbengh commented Sep 10, 2019

What does it do?

While working on hexojs/hexo-generator-sitemap#68 and hexojs/hexo-generator-feed#93, I find it strange that plugins have to encode url themselves.

I believe url should be safe to consume by default. I can't think of any reason to use this.url in its original unescape form, except for aesthetic when displaying as a text, e.g. show http://foo.com/一二三 instead of http://foo.com/%E4%B8%80%E4%BA%8C%E4%B8%89

Displaying a full link as text should be considered as an edge case. In that case, it should be up to the user to decode it (we could write a helper to help).

Do note link inside href is decoded when mouse-over.

<a href="http://xn--fo-8ja.com/b%C3%A1r%20ab/">aaaaaaaaaaaaa</a>

link

It's a breaking change because existing plugins need to remove url encode function, otherwise the percent symbol gets encoded (double-encoded), e.g. foo%20bar becomes foo%25%20bar.

Note that existing plugin can remain backward compatible with v3 (if they wish) by using encodeURI(decodeURI(str)) (instead of current encodeURI(str), or use encodeURL() of hexo-util.

hexo-util is going to have decodeURL() which is an alternative to decodeURI with added ability to decode punycoded IDN.

How to test

git clone -b encode-url https://github.com/curbengh/hexo.git
cd hexo
npm install
npm test

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.
@coveralls
Copy link

@coveralls coveralls commented Sep 10, 2019

Coverage Status

Coverage increased (+0.006%) to 97.236% when pulling e91aa2e on curbengh:encode-url into ab90487 on hexojs:master.

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 10, 2019

I also notice an unusual behavior in tag/category name. It's automatically converted to ASCII when slugize as path, e.g. bár to bar. Maybe it is intended by slugize.

@curbengh curbengh requested a review from Sep 16, 2019
@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 16, 2019

Can this be part of v4? Since this is a breaking change, if this PR misses v4, it would have to be in v5 😢 .

@tomap
Copy link
Contributor

@tomap tomap commented Sep 16, 2019

I'm not sure If fully agree with this change. Because, the htmldncode should be done when ln you know that you are going to output this url into html, this at the end, in each plugin. A plugin could produce txt. And it would not be nice to ask to decode.
However I understand the intention. And this is a security feature. To be complete, it would be.important to have a raw url available, and a raw permalink. But I'm not sure if there is a need for that

@curbengh
Copy link
Contributor Author

@curbengh curbengh commented Sep 16, 2019

A plugin could produce txt. And it would not be nice to ask to decode.

That's a valid use case and a new decodeURL (to be implemented if we're going ahead with this) helper can help.

I do, however, consider that as a minor use case. Majority of the url and permalink end up in html, as such I propose to encode by default. Besides, it would be nice not to ask plugin/theme devs (outside of the aforementioned use case) to encode themselves.

Of course, my opinion could be uninformed. @theme-next is this a breaking change for theme?


Note that existing plugin can remain backward compatible with v3 (if they wish) by using encodeURI(decodeURI(str)) (instead of current encodeURI(str), or use encodeURL() of hexo-util.

@curbengh curbengh added this to the v4.0.0 milestone Sep 18, 2019
lib/models/post.js Outdated Show resolved Hide resolved
lib/models/page.js Outdated Show resolved Hide resolved
lib/models/tag.js Outdated Show resolved Hide resolved
@@ -22,7 +23,7 @@ function urlForHelper(path = '/', options) {

// Resolve relative url
if (options.relative) {
return this.relative_url(this.path, path);
return relative_url(this.path, path);
Copy link
Contributor Author

@curbengh curbengh Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fix is already present in https://github.com/hexojs/hexo/pull/3710/files#diff-d89aa048c56f2709ea59155d3f91aa81 , I put this as not to depend #3710

@curbengh curbengh requested a review from SukkaW Sep 19, 2019
SukkaW
SukkaW approved these changes Sep 19, 2019
@curbengh curbengh merged commit a58acb9 into hexojs:master Sep 20, 2019
3 of 4 checks passed
@curbengh curbengh deleted the encode-url branch Sep 21, 2019
@curbengh curbengh changed the title fix: encode url and permalink by default fix: encode permalink by default Oct 9, 2019
stevenjoezhang added a commit to theme-next/hexo-theme-next that referenced this issue Dec 25, 2019
sslogan666 added a commit to sslogan666/sslogan666.github.io that referenced this issue Jun 12, 2021
mapxn added a commit to wnwd/hexo-theme-next that referenced this issue Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants