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

empty charset fix #1300

Merged
merged 1 commit into from
Jun 4, 2019
Merged

empty charset fix #1300

merged 1 commit into from
Jun 4, 2019

Conversation

ahorek
Copy link
Contributor

@ahorek ahorek commented Dec 18, 2018

fixes #1299

@ahorek
Copy link
Contributor Author

ahorek commented Dec 19, 2018

the actual problem is in action mailer, that always expects a charset to be defined and #1145 broke it, however I think it should be fixed here, otherwise it'll break older rails versions

@mikel what about add action mailer's tests to travis?

@jeremy
Copy link
Collaborator

jeremy commented Dec 19, 2018

@ahorek, that's a good idea. Do Action Mailer tests fail with #1145? We could test Action Mailer against Mail master branch as well.

@ahorek
Copy link
Contributor Author

ahorek commented Dec 19, 2018

Do Action Mailer tests fail with #1145?

yes, they do fail. I'm working on CI integration in #1303

@jeremy
Copy link
Collaborator

jeremy commented Dec 19, 2018

Great! Let's get a PR going in Rails as well. We can track Mail master branch in an additional CI job there.

if value
@charset = value
@header.charset = value
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would someone remove the charset now, to rely on default parsing?

This is seeming like something to fix downstream in Action Mailer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it isn't just "bad tests", it breaks Action Mailer for real. Even if it'll be fixed in Action Mailer, Mail 2.8 will be incompatible with all previous Rails versions, I think we should avoid that for all costs.

what about skipping blank parameters? https://github.com/mikel/mail/blob/master/lib/mail/fields/parameter_hash.rb#L47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I fixed that by removing the existing header if you set charset = nil

note that this case didn't work even on previous mail versions including 2.7.1 stable

@ahorek
Copy link
Contributor Author

ahorek commented Mar 6, 2019

if you have time, please review #1308 #1301 these two PRs are preventing a green build on travis

@ahorek
Copy link
Contributor Author

ahorek commented Jun 3, 2019

ping @jeremy

it should be ready, what do you think? any objections? travis build is still failing because of #1308

@jeremy jeremy added this to the 2.7.2 milestone Jun 4, 2019
@jeremy jeremy merged commit b2a7c72 into mikel:master Jun 4, 2019
jeremy pushed a commit that referenced this pull request Jun 4, 2019
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.

[master] missing charset regression in action mailer
2 participants