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(encodeURL): correct URL search parameter encoding #413

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

uiolee
Copy link
Member

@uiolee uiolee commented Feb 8, 2025

check list

  • Add test cases for the changes.
  • Passed the CI test.

Description

fix #411

Additional information

@coveralls
Copy link

coveralls commented Feb 8, 2025

Coverage Status

coverage: 96.875%. remained the same
when pulling e6c067e on uiolee:searchparam
into 1ad96ca on hexojs:master.

@axiac
Copy link

axiac commented Feb 12, 2025

The code currently uses four sets of URL handling components:

The functions/classes in each of these groups work fine together but combining them across groups produces unexpected (and sometimes invalid) results because their functionalities do not overlap.

What's the purpose of this decoding and encoding?
It only introduces confusion and bugs.

I would avoid the operations that should cancel each other (encodeURI(decodeURI(...))) and stick to a single set of functions/classes to manipulate the URLs.
Since url.parse() and url.format() are deprecated and encodeURI()/decodeURI() deal with the whole URL at once and do not play well with decodeURIComponent()/querystring.unescape(), I would suggest to use only class URL to validate, parse and possibly re-encode the input URL correctly.
Its documentation describes what it does and it seems to cover all transformations and fixes that function encodeURL() does when it attempts to fix a badly encoded URL.

@uiolee
Copy link
Member Author

uiolee commented Feb 14, 2025

@axiac

Thanks for your advice.

I also don't know the purpose of some of the code.
They may solve certain problems.
It may also be a historical issue, at least hexo was born before WHATWG URL were added to node.

However, hexo-util is referenced by other packages, so it has to be changed with caution

@SukkaW SukkaW requested review from SukkaW and a team and removed request for SukkaW February 17, 2025 01:13
Copy link
Member

@renbaoshuo renbaoshuo left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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.

encodeURL() rewrites the query string incorrectly
5 participants