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

Add fallback to ISO 8601 date format in ContentDispositionParser. #554

Merged

Conversation

reitermarkus
Copy link
Contributor

Some websites incorrectly use ISO 8601 for the dates, causing an exception to be raised and rendering all other correctly specified fields unusable.

@reitermarkus
Copy link
Contributor Author

Ping?

@flavorjones
Copy link
Member

Hi, thanks for proposing this change, and apologies you haven't gotten any feedback until now.

There are tests covering the behavior of Mechanize::HTTP::ContentDispositionParser in test/test_mechanize_http_content_disposition_parser.rb, but notably they do not address the parser's behavior when a header is encountered that does not follow RFC 2183 to use RFC 822 dates.

I'd be open to merging this if I saw, as part of this PR:

  • a test describing current behavior when given an unparseable date string
  • test describing the new behavior when using ISO8601 date strings
  • and the first test should continue to pass before and after the changes to ContentDispositionParser

Thanks for being open to this request.

@reitermarkus
Copy link
Contributor Author

a test describing current behavior when given an unparseable date string

The current behavior is that it raises an ArgumentError, however, it should return nil given that all other wrong fields also don't result in an error but nil being returned.

@reitermarkus
Copy link
Contributor Author

@flavorjones, I added some tests. The other test failures seem unrelated.

@flavorjones
Copy link
Member

Hi @reitermarkus, thanks for adding the test coverage. The change of behavior (from raising an exception to returning nil) makes sense to me (though I'm not sure why the current implementation returns nil rather than simply omitting the key or value from the returned hash -- that's for another day).

The existing test failures are ... well, that's not great. Mechanize has not received much attention from maintainers recently. I'll try to set aside some time over the holidays to try to get the tests in shape again.

@flavorjones
Copy link
Member

I just rebased off master, let's see what CI thinks now.

@flavorjones flavorjones added this to the v2.8.0 milestone Feb 1, 2021
Base automatically changed from master to main February 7, 2021 19:22
@flavorjones
Copy link
Member

Looks good, merging. Will be in the next release, v2.8.0.

@flavorjones flavorjones merged commit 798be50 into sparklemotion:main Apr 1, 2021
@reitermarkus reitermarkus deleted the content-disposition-date branch April 3, 2021 21:51
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.

None yet

2 participants