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) #521 trim leading and trailing empty values in parse_qs #523

Merged
merged 3 commits into from Sep 10, 2018

Conversation

Projects
None yet
3 participants
@leonardb
Contributor

leonardb commented Aug 21, 2018

Fixes crash in parse_qs when qs contains extra ampersands

@benoitc

This comment has been minimized.

Owner

benoitc commented Aug 30, 2018

when trim_all has been added to OTP? Was it always there?

@leonardb

This comment has been minimized.

Contributor

leonardb commented Aug 30, 2018

Looks like it was added in 18. If this still needs to be compatible with < 18 we can change

@benoitc

This comment has been minimized.

Owner

benoitc commented Aug 30, 2018

@benoitc benoitc force-pushed the benoitc:master branch from d6b0b05 to 499de03 Sep 1, 2018

@edgurgel

This comment has been minimized.

Collaborator

edgurgel commented Sep 4, 2018

@leonardb

This comment has been minimized.

Contributor

leonardb commented Sep 6, 2018

I'm not sure if we can achieve this reliably and safely using a regex. If we want to do this safely I could always extract the binary:split/3 logic/code from 18 into a local function as a stop-gap until you decide to formally drop support for < 18

@benoitc

This comment has been minimized.

Owner

benoitc commented Sep 10, 2018

If we want to do this safely I could always extract the binary:split/3 logic/code from 18 into a local function as a stop-gap until you decide to formally drop support for < 18

Indeed. Doing it would allows us to include the change in 1.x directly, with a proper note to remove that hack in coming 2.x release.

@edgurgel hackney is still supposed to work on 17.x and sup. even if not advised. That support will be officially removed in 2.x .

@leonardb

This comment has been minimized.

Contributor

leonardb commented Sep 10, 2018

@benoitc Which module would you prefer this to be in?

  1. hackney_bstr.erl
  2. hackney_util.erl
  3. hackney_url.erl
@benoitc

This comment has been minimized.

Owner

benoitc commented Sep 10, 2018

Right :) I think hackney_bstr is the right place for it.

@leonardb

This comment has been minimized.

Contributor

leonardb commented Sep 10, 2018

@benoitc Ready for review

@benoitc benoitc merged commit b98f89d into benoitc:master Sep 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benoitc

This comment has been minimized.

Owner

benoitc commented Sep 10, 2018

@leonardb thanks!, changes looked good, merged it :) release will happen later today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment