urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities

[papandreou]

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 custom decodeURIComponent implementation to use wasn't added until 0.12.

[dougwilson]

Note: This will fail in node.js 0.8 and 0.10 as the capability to specify a custom decodeURIComponent implementation to use wasn't added until 0.12.

We can hold the PR for the 2.0 release, which will drop support for those versions 👍

[papandreou]

@dougwilson, that sounds fair! Does it look good otherwise?

[Lurow]

Hi there,

Any news on v2.0 release date with the pr?

Cheers

[thopaw]

Any news to iso-8859-1 support?

[papandreou]

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 .travis.yml if that would be a help? (I guess it might be a nice occasion to drop more old versions)

[dougwilson]

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.

[papandreou]

Okay, thanks for the update, that makes sense!

If this could be made to work in those node.js versions I would of course be happy to land right away.

Unfortunately I think this would involve us to stop relying on the built-in querystring module in simple mode. Either by copying in a newer implementation than the one in 0.10, or by figuring out a way to use the qs module in simple mode as well, disabling the advanced behaviors. That seems a bit far-reaching, unless you were already planning to go in that direction?

[ljharb]

@papandreou if there's manageable changes i could make in qs that would allow body-parser to switch to it for everything, i'm willing to make those.

[dougwilson]

And I would be happy to use qs for everything here as well. The downside to using querystring for the simple parser is that since it ships as part of Node.js core, the results of this module will vary by both the version of this module and the version of Node.js (vs only varying by the version of this module).

[ljharb]

@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 🙏

[dougwilson]

@ljharb this is likely not even correct for params to qs (I passed depth: -1 since 0 is not the right behavior, but idk), but it's at least something that should help :)

Branch: https://github.com/expressjs/body-parser/tree/urlencoded-qs-simple
Test: npm i && npm test -- --grep=bodyParser.urlencoded

[ljharb]

give me 48 hours, i'll see what i can do

[ljharb]

body-parser tests on that branch now pass with ljharb/qs#326 (comment), and I can merge and release that in qs as soon as I've convinced myself what semver it is.

[dougwilson]

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 :)

[ljharb]

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.

[ljharb]

v6.8.0 of qs is published.

[filecage]

The PR seems to be ready and all dependencies resolved. Is there anything left blocking this PR to be merged (except for the package.json conflict) and released as minor that I can help with?

Even in 2020 it is still interesting to have ISO-8859-1 support in web apps. 😬

[ljharb]

might be good to make sure it's using the latest qs, v6.9.4, but otherwise i'd still love to see this land as well.

[papandreou]

Resolved the conflict and updated to 6.9.4 😇

[DarkBitz]

Is there a specific reason why the pull request is not being merged?