Do not remove empty GET/POST parameters #585

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@ysimonson
Contributor

ysimonson commented Aug 16, 2012

Is there a reason why tornado currently removes empty GET/POST parameters? It is causing issues with validating forms generated by formalchemy, because formalchemy expects GET/POST parameters to be there, even if they are empty.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 16, 2012

This pull request passes (merged ebf17aa into 2b07385).

This pull request passes (merged ebf17aa into 2b07385).

@superduper

This comment has been minimized.

Show comment
Hide comment
@superduper

superduper Aug 16, 2012

Especially useful for OAuth basestring calculation, when you need list of all parameters even if they have empty values.

Especially useful for OAuth basestring calculation, when you need list of all parameters even if they have empty values.

@bdarnell

This comment has been minimized.

Show comment
Hide comment
@bdarnell

bdarnell Aug 27, 2012

Member

This has come up several times before and I think it's a good change (with the exception that I'm not sure whether the singular RequestHandler.get_argument should treat empty arguments as unspecified). It is potentially backwards-compatibile, though, so I'd like to hold off until tornado 3.0 (which is not that far off - I'm going to do a 2.4 release soon and then next will probably be 3.0).

Member

bdarnell commented Aug 27, 2012

This has come up several times before and I think it's a good change (with the exception that I'm not sure whether the singular RequestHandler.get_argument should treat empty arguments as unspecified). It is potentially backwards-compatibile, though, so I'd like to hold off until tornado 3.0 (which is not that far off - I'm going to do a 2.4 release soon and then next will probably be 3.0).

@kung-foo

This comment has been minimized.

Show comment
Hide comment
@kung-foo

kung-foo Oct 18, 2012

FYI, this pull does not enable empty POST parameters. I've submitted a pull here with the update: facebook#614

FYI, this pull does not enable empty POST parameters. I've submitted a pull here with the update: facebook#614

@superduper

This comment has been minimized.

Show comment
Hide comment
@superduper

superduper Oct 18, 2012

@kung-foo nice catch, but still incomplete :P your pull does not enable empty POST parameters for requests with Content-Type multipart/form-data

@kung-foo nice catch, but still incomplete :P your pull does not enable empty POST parameters for requests with Content-Type multipart/form-data

@kung-foo

This comment has been minimized.

Show comment
Hide comment
@kung-foo

kung-foo Oct 18, 2012

ah yes. now I remember starting to look into the multipart processing code and thinking, wtf. nope. 😐

ah yes. now I remember starting to look into the multipart processing code and thinking, wtf. nope. 😐

@superduper

This comment has been minimized.

Show comment
Hide comment
@superduper

superduper Oct 18, 2012

@kung-foo false alarm 😊 your patch seems to be fine. I've checked again parse_multipart_form_data implementation and figured out that there is no filter for empty values on multipart data forms. maybe need to add tests for this case.

@kung-foo false alarm 😊 your patch seems to be fine. I've checked again parse_multipart_form_data implementation and figured out that there is no filter for empty values on multipart data forms. maybe need to add tests for this case.

Rudd-O pushed a commit to Rudd-O/tornado that referenced this pull request Jun 27, 2013

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