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

[MRG+1] Fix form methods in FormRequest.from_response (#3777) #3794

Merged
merged 3 commits into from
Jul 2, 2019
Merged

[MRG+1] Fix form methods in FormRequest.from_response (#3777) #3794

merged 3 commits into from
Jul 2, 2019

Conversation

csalazar
Copy link
Contributor

@csalazar csalazar commented May 25, 2019

Fixes #3777

@csalazar csalazar changed the title Whitelist form methods in FormRequest.from_response (#3777) Fix form methods in FormRequest.from_response (#3777) May 25, 2019
Gallaecio
Gallaecio previously approved these changes May 27, 2019
@Gallaecio Gallaecio changed the title Fix form methods in FormRequest.from_response (#3777) [MRG+1] Fix form methods in FormRequest.from_response (#3777) May 27, 2019

method = kwargs.pop('method', form.method).upper()
if method not in cls.valid_form_methods:
raise ValueError('Invalid form method in chosen form')
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should follow standard more closely, and consider unknown and invalid methods as GET (see https://www.w3.org/TR/html5/sec-forms.html#element-attrdef-form-method):

The invalid value default for these attributes is the GET state. The missing value default for the method attribute is also the GET state. (There is no missing value default for the formmethod attribute.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I agree with that, I'm going to modify the PR.

@Gallaecio Gallaecio changed the title [MRG+1] Fix form methods in FormRequest.from_response (#3777) Fix form methods in FormRequest.from_response (#3777) May 31, 2019
@Gallaecio Gallaecio dismissed their stale review May 31, 2019 06:16

See Mikhail’s feedback

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #3794 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3794      +/-   ##
==========================================
+ Coverage   85.42%   85.43%   +<.01%     
==========================================
  Files         169      169              
  Lines        9635     9638       +3     
  Branches     1433     1434       +1     
==========================================
+ Hits         8231     8234       +3     
  Misses       1156     1156              
  Partials      248      248
Impacted Files Coverage Δ
scrapy/http/request/form.py 96.12% <100%> (+0.09%) ⬆️

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #3794 into master will decrease coverage by 0.02%.
The diff coverage is 80%.

@@            Coverage Diff            @@
##           master   #3794      +/-   ##
=========================================
- Coverage   85.42%   85.4%   -0.03%     
=========================================
  Files         169     169              
  Lines        9635    9687      +52     
  Branches     1433    1445      +12     
=========================================
+ Hits         8231    8273      +42     
- Misses       1156    1166      +10     
  Partials      248     248
Impacted Files Coverage Δ
scrapy/http/request/form.py 95.41% <80%> (-0.62%) ⬇️
scrapy/commands/parse.py 20.32% <0%> (-0.73%) ⬇️
scrapy/commands/check.py 25.35% <0%> (-0.37%) ⬇️
scrapy/http/response/text.py 97.84% <0%> (ø) ⬆️
scrapy/crawler.py 92.39% <0%> (ø) ⬆️
scrapy/http/request/__init__.py 100% <0%> (ø) ⬆️
scrapy/core/scraper.py 88.51% <0%> (ø) ⬆️
scrapy/settings/default_settings.py 98.65% <0%> (ø) ⬆️
scrapy/downloadermiddlewares/redirect.py 96.82% <0%> (+0.05%) ⬆️
scrapy/loader/__init__.py 94.59% <0%> (+0.07%) ⬆️
... and 4 more

@csalazar
Copy link
Contributor Author

csalazar commented Jun 6, 2019

Updated with Mikhail's feedback

@Gallaecio Gallaecio changed the title Fix form methods in FormRequest.from_response (#3777) [WIP] Fix form methods in FormRequest.from_response (#3777) Jun 7, 2019
@Gallaecio Gallaecio changed the title [WIP] Fix form methods in FormRequest.from_response (#3777) [MRG+1] Fix form methods in FormRequest.from_response (#3777) Jun 7, 2019
scrapy/http/request/form.py Outdated Show resolved Hide resolved
@kmike kmike added this to the v1.7 milestone Jun 27, 2019
@kmike kmike merged commit 9aec785 into scrapy:master Jul 2, 2019
@kmike
Copy link
Member

kmike commented Jul 2, 2019

Thanks @csalazar!

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

Successfully merging this pull request may close these issues.

Whitelist form methods in FormRequest.from_response
3 participants