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 Urlencoded for nested Hashes (and Arrays) #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexWayfer
Copy link
Contributor

@AlexWayfer AlexWayfer commented Jul 6, 2018

Let's unify this code!

rack/rack#1281


This change is Reviewable

@ixti
Copy link
Member

ixti commented Jul 8, 2018

One of the goals of this gem is to be free of dependencies and rovide everything on it's own. Basically we use URI right now because nobody had time to implement proper serializer. :D

@AlexWayfer
Copy link
Contributor Author

AlexWayfer commented Jul 8, 2018

One of the goals of this gem is to be free of dependencies and rovide everything on it's own.

Why? For what?

Basically we use URI right now because nobody had time to implement proper serializer. :D

URI has problems with nested Hashes and Arrays, you want to write own "proper" serializer. Rack already has its own serializer, you want to have, Addressable (possible) have (or had). Instead of multiple different implementations (some of which can be not able to catch some edge cases) I suggest to use one polished standard code (library).

@ixti
Copy link
Member

ixti commented Jul 8, 2018

Basically because this gem was born to encapsulate all the needed logic of serializing form data into multipart or urlencoded. I'm not saying I like using URI - I said we're using it just because we did not spend time to come with better solution. Using extra dependency for a very own purpose sound to me pretty bad practice and pretty smells like leftpad of node.js.

That's just my personal perception.

In other words - I'll be happy to consider PR that will bring provide better default encoder as part of FormData implementation.

@AlexWayfer AlexWayfer force-pushed the fix_for_nested_hashes branch 2 times, most recently from 377885b to c713a12 Compare July 9, 2018 22:08
@AlexWayfer
Copy link
Contributor Author

Done…

@AlexWayfer
Copy link
Contributor Author

Wow, floating bug from global state. I hate global state.

@AlexWayfer AlexWayfer force-pushed the fix_for_nested_hashes branch from ef7bd9c to f5386d8 Compare July 10, 2018 15:50
@pcriv
Copy link

pcriv commented May 15, 2019

Are the plans to merge this? 😅

@ixti
Copy link
Member

ixti commented Mar 4, 2020

Sorry for a very slow reaction. I'll try to merge it this week.

@bruno-
Copy link

bruno- commented Sep 15, 2021

I think I'm having an issue that this PR would fix.
Basically, I'm trying to form encode an array and it doesn't work right:

HTTP::FormData.create({foo: ["bar", "baz"]}).to_s # => "foo=bar&foo=baz"

The correct output would be foo%5B%5D=bar&foo%5B%5D=baz.
The temporary, dirty fix can be HTTP::FormData.create({"foo[]": ["bar", "baz"]}).to_s (note the [] after foo)

@bacarini
Copy link

Sorry for a very slow reaction. I'll try to merge it this week.

@ixti do you have any plan to merge this soon? we are having a similar problem because the nested hash

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.

5 participants