-
-
Notifications
You must be signed in to change notification settings - Fork 735
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] parsing comma delimited str with percent encoded comma #336
Conversation
… treated as normal text Co-authored-by: Mohamed Omar <[email protected]> Co-authored-by: Quentin de Longraye <[email protected]>
lib/parse.js
Outdated
} | ||
|
||
if (val && options.interpretNumericEntities && charset === 'iso-8859-1') { | ||
val = interpretNumericEntities(val); | ||
} | ||
|
||
if (val && typeof val === 'string' && options.comma && val.indexOf(',') > -1) { | ||
val = val.split(','); |
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.
Removing this is a breaking change; it forces anyone using a custom decoder to add this logic where they previously didn't require it.
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.
i agree, i will look into it more to see what can be done
You can pull a non-breaking version here: https://github.com/quentinus95/qs/tree/comma-perecent-parsing |
c45429d
to
8aa59b6
Compare
@quentinus95 pulled in and rebased, but now the added test is failing. |
@quentinus95 that was added in #359, and is a pretty important bugfix - so this PR may have to be adapted to work with it. |
@ljharb thanks, I've just had a look over it, and it would take too long for me to support the fix (especially because #359 has duplicated some of the logic). I've started to do some refactoring but won't have time to achieve it. If you have some hints on how to fix this without rewriting 20% of the parser, I'll be happy to write the implementation. |
Looks like just the test needs to be updated. |
8aa59b6
to
cd9a3cd
Compare
@@ -429,6 +429,14 @@ test('parse()', function (t) { | |||
st.end(); | |||
}); | |||
|
|||
t.test('parses comma delimited array while having percent-encoded comma treated as normal text', function (st) { | |||
st.deepEqual(qs.parse('foo=a%2Cb', { comma: true }), { foo: ['a', 'b'] }); |
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.
@ljharb this test is wrong, it should return "a,b". It's the main point of the pull request and the related issue.
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.
ah, i understand. my bad.
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.
Filed #361 to proceed with a proper fix.
… treated as normal text (#336) Co-authored-by: Mohamed Omar <[email protected]> Co-authored-by: Quentin de Longraye <[email protected]>
… treated as normal text (#336) Co-authored-by: Mohamed Omar <[email protected]> Co-authored-by: Quentin de Longraye <[email protected]>
changes on the default decoder to correctly parse comma delimited strings while having percent encoded comma
Fixes #311.