-
Notifications
You must be signed in to change notification settings - Fork 702
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
urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326
urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326
Conversation
…d also accept iso-8859-1 as a default encoding.
Works in both extended and simple mode.
c056515
to
5b39b86
Compare
We can hold the PR for the 2.0 release, which will drop support for those versions 👍 |
@dougwilson, that sounds fair! Does it look good otherwise? |
Hi there, Any news on v2.0 release date with the pr? Cheers |
Any news to iso-8859-1 support? |
Fixed conflict with master. @dougwilson, this has been sitting for some time now 😅 I'm not really in a hurry, but how about getting this released soon? I can remove those ancient node versions from |
The reason it has been sitting is just because it cannot be merged into the 1.x line due to the incompatibility. Because this module is a part of express, it effectively inherits it's support policy. The 1.x series of this module won't end up out of support for some time, and the count down starts with the next express release. In order to reduce the support burden by maintaining multiple major versions of this module, I am just waiting to make the 2.0 in coordination with express 5. If this could be made to work in those node.js versions I would of course be happy to land right away. |
Okay, thanks for the update, that makes sense!
Unfortunately I think this would involve us to stop relying on the built-in |
@papandreou if there's manageable changes i could make in |
And I would be happy to use |
@dougwilson if you can save me a few minutes and link me to a branch that uses qs for the simple parser, but is failing tests, i can use npm link and see what options and changes it would take to get it working :-D 🙏 |
@ljharb this is likely not even correct for params to Branch: https://github.com/expressjs/body-parser/tree/urlencoded-qs-simple |
give me 48 hours, i'll see what i can do |
body-parser tests on that branch now pass with ljharb/qs#326 (comment), and I can merge and release that in |
Ah, yes, so depth: 0 was supposed to make sense after all :D . Yes, that is a hard decision. But if it helps at all, this module and Express do not directly expose the qs arguments, making whichever semver it is a non issue to us :) |
What I'm probably leaning towards is releasing it as a minor, and if users report breakage, reverting the depth 0 part, but keeping the depth false part - so express thus should use depth false to ensure continuity. |
v6.8.0 of qs is published. |
0ad1d88
to
2a2f471
Compare
Is there a specific reason why the pull request is not being merged? |
* Always use the qs module, even in simple mode #326 (comment) * Create the simple and extended parser with the same function, reducing duplication * Don't mention the querystring module in the README * Fix lint
I merged #387 yesterday, I am trying to catch up on what was going on here but was thinking it might be a better idea to see if you (@papandreou) wanted to get this PR updated with that in mind? Or are we calling this not necessary anymore? |
means it should probably wait for a major? |
A major is what I am doing! So that is why I am circling back on all these PRs. EDIT: I should have pointed out the "changed the base branch from master to 2.x" |
in that case it seems like a great idea |
With that in mind, I am not up on the specifications involved here and am just doing the mechanical turk job of preparing all these stale branches for prime time, can you give a review and TLDR of what we should be aware of with this and #387 going out along with |
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.
Reviewed some of it, but i'm not familiar enough with body-parser to review the rest.
README.md
Outdated
##### defaultCharset | ||
|
||
The default charset to parse as, if not specified in content-type. Must be | ||
either `utf-8` or `iso-8859-1`. |
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.
this allows changing the charset, which is a nice semver-minor; the default should probably be utf-8
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.
It is. Documented that in 44a6aa9 now.
Whether to let the value of the `utf8` parameter take precedence as the charset | ||
selector. It requires the form to contain a parameter named `utf8` with a value | ||
of `✓`. Defaults to `false`. |
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.
this goes along with defaultCharset
##### interpretNumericEntities | ||
|
||
Whether to decode numeric entities such as `☺` when parsing an iso-8859-1 | ||
form. Defaults to `false`. |
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.
and this goes along with the other defaultCharset value
lib/types/urlencoded.js
Outdated
var charsetBySentinel = { | ||
// This is what browsers will submit when the ✓ character occurs in an | ||
// application/x-www-form-urlencoded body and the encoding of the page containing | ||
// the form is iso-8859-1, or when the submitted form has an accept-charset | ||
// attribute of iso-8859-1. Presumably also with other charsets that do not contain | ||
// the ✓ character, such as us-ascii. | ||
'✓': 'iso-8859-1', // encodeURIComponent('✓') | ||
// These are the percent-encoded utf-8 octets representing a checkmark, indicating | ||
// that the request actually is utf-8 encoded. | ||
'✓': 'utf-8' // encodeURIComponent('✓') | ||
} | ||
|
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.
this does confuse me a bit tho; it kind of seems like code that already exists in qs? meaning that the options can just be passed to qs and it'll handle the logic.
altho if someone's using a different querystring parser then the logic might need replicating
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.
You're correct that these magic values are also found here. I spent some time trying to remember why also having the decoding logic here was warranted. It seems like we can get actually get rid of it and still have the test suite pass!
I wonder if I did it like this because it seemed like a long shot to get changes into the qs
module, so I went for body-parser
first, because that's what I needed myself. It has been 6 years, though, so I'm not really sure 🤔
* 2.x: (98 commits) fix(deps): raw-body@^3.0.0 (expressjs#529) Also use the qs module for the simple parser (expressjs#387) feat!: remove node less than 18 from ci 2.0.0-beta.2 docs: add missing history entry tests: enable strict mode Remove deprecated bodyParser() combination middleware build: remove conditional code coverage deps: [email protected] deps: [email protected] deps: [email protected] 1.20.2 Fix strict json error message on Node.js 19 deps: [email protected] build: [email protected] build: [email protected] build: [email protected] deps: content-type@~1.0.5 build: [email protected] build: [email protected] ...
14fb806
to
9a4ebac
Compare
I think it's still a nice-to-have! I've merged the |
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.
LGTM - altho i don’t actually see how this is breaking? where is a custom decodeURIComponent implementation used?
Ah, that referred to the Before #387, |
Well either way I don't think we are doing any extra work to land these things in the current majors. We have needed to clean house on much of this for a while, so as long as this is good to land we will be doing it along with a major no matter what. And with that in mind, here I am clicking the merge button. We will be preparing the v2 release soon. |
Fixes #194
Further elaborations here: ljharb/qs#268
Supports both the simple and extended parsers as outlined in ljharb/qs#268 (comment) -- and doesn't use the equivalent capabilities of the qs module.
Note: This will fail in node.js 0.8 and 0.10 as the capability to specify a customdecodeURIComponent
implementation to use wasn't added until 0.12.