Skip to content

Allow form.submit with url string param to use https#249

Merged
alexindigo merged 2 commits intoform-data:masterfrom
vmosyaykin:master
Jun 26, 2017
Merged

Allow form.submit with url string param to use https#249
alexindigo merged 2 commits intoform-data:masterfrom
vmosyaykin:master

Conversation

@vmosyaykin
Copy link
Copy Markdown
Contributor

I want to be able to pass a string starting with https: to form.submit, and I expect the form to be submitted over https. url.parse returns the protocol property, however previously it was ignored.

I also added some tests, and would like to hear some comments on them. I had to write them a bit differently from the others, because you can't pass options for self-signed certs when using string as first parameter to form.submit.

@review-ninja
Copy link
Copy Markdown

ReviewNinja

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 29, 2016

Coverage Status

Coverage remained the same at 96.316% when pulling 4043fbb on vmosyaykin:master into 158443d on form-data:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 29, 2016

Coverage Status

Coverage remained the same at 96.316% when pulling 54bf849 on vmosyaykin:master into 158443d on form-data:master.

2 similar comments
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 96.316% when pulling 54bf849 on vmosyaykin:master into 158443d on form-data:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 96.316% when pulling 54bf849 on vmosyaykin:master into 158443d on form-data:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 29, 2016

Coverage Status

Coverage remained the same at 96.316% when pulling b4be5c5 on vmosyaykin:master into 158443d on form-data:master.

@vmosyaykin
Copy link
Copy Markdown
Contributor Author

Sorry for a messy PR. Fixed my test to pass on node 0.10.x

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 20, 2016

Coverage Status

Coverage remained the same at 97.861% when pulling adbfa70 on vmosyaykin:master into 652b16f on form-data:master.

2 similar comments
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 97.861% when pulling adbfa70 on vmosyaykin:master into 652b16f on form-data:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 97.861% when pulling adbfa70 on vmosyaykin:master into 652b16f on form-data:master.

@vmosyaykin
Copy link
Copy Markdown
Contributor Author

Squashed my commits into one for nicer history

Copy link
Copy Markdown
Member

@alexindigo alexindigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking care of it. I have couple suggestions.

req = form.submit('http://localhost:' + common.port + '/path', function() {});
assert.strictEqual(req.path, '/path');
assert.ok(req.agent instanceof http.Agent, 'req.agent instanceof http.Agent');
assert.strictEqual(req.getHeader('Host'), 'localhost:' + common.port);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add similar check to the https counter part?
And maybe try with default ports, since you're going through with the request anyway.

Sounds good?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I'll push fixes once the comment above is resolved.


// Basic parsing
req = form.submit('http://localhost:' + common.port + '/path', function() {});
assert.strictEqual(req.path, '/path');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, what are you referring to. The /path assertation?

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 8, 2017

Coverage Status

Coverage remained the same at 97.927% when pulling eec0e80 on vmosyaykin:master into 03444d2 on form-data:master.

@vmosyaykin
Copy link
Copy Markdown
Contributor Author

@alexindigo sorry for the delay. I improved the tests a bit.

@zackbloom
Copy link
Copy Markdown

👍

@AWare
Copy link
Copy Markdown

AWare commented Jun 23, 2017

Is there a plan to release this?
Silently downgrading to http is pretty concerning.

@alexindigo
Copy link
Copy Markdown
Member

Thank you @vmosyaykin

@alexindigo alexindigo merged commit 9bfb102 into form-data:master Jun 26, 2017
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.

6 participants