remove duplicate field stripping #766

grosser · 2014-07-31T23:10:42Z

calling afield with it's own name seems like a pretty useless feature, removing this breaks ~30 specs, that I'd fix if we decide to go this way ...

#720

Ideally the .to_s would not be necessary, but something in mail or action_mailer passes in a class for content_transfer_encoding :/

fix:

# https://github.com/mikel/mail/pull/766
Mail::CommonField.class_eval do
  def strip_field(field_name, value)
    value.to_s
  end
end

jeremy · 2014-08-01T01:25:38Z

Agree, but it's a breaking API change. We could introduce a new API to instantiate a field with a value only, like Field.build value, or change #initialize to take either a string or value: ... argument.

The current Field.new "header: value" API may be deprecated and either removed or moved to a Field.parse ... API.

kjg · 2014-08-01T02:18:22Z

Does this completely remove the need for strip_field? If so, #719 might not be needed if this gets accepted.

grosser · 2014-08-01T04:32:27Z

yes, this kills strip_field

I never saw anyone use the field api directly, mostly either parsing whole mails or building them via actionmailer, so not sure if that's a real use-case. It's also very easy to change offending code to be Field.new "Name", "Value" instead of `"Name: Value", right ?

grosser · 2014-08-01T04:32:46Z

all green now (at least locally)

grosser · 2014-08-06T21:05:36Z

Anything missing here ?

jeremy · 2014-08-07T01:31:43Z

You'll need to deprecate the API—or introduce new API—rather than making a breaking change.

jeremy · 2014-08-07T01:35:05Z

lib/mail/envelope.rb

Note that strip_field used to always return a string. Now we may be supering with a range of values that we didn't design for (or test).

grosser · 2014-08-08T00:02:42Z

made all values || "" to not influence the other code

not sure why we need any new api, this seems to work fine:

>> Mail::Field.new "Subject", "Fppp"
=> #<Mail::Field 0x7fec1d8304f8 @name="Subject" @value="Fppp" @raw_value=nil @charset="utf-8">

also very few people should be using Field directly ...

grosser · 2014-08-12T02:44:11Z

does this need more or can it get merged ?

grosser · 2014-08-16T04:20:13Z

@jeremy what do I need to do to get this merged ?

jeremy · 2014-08-26T23:02:45Z

You'll need to deprecate the API—or introduce new API—rather than making a breaking change.

grosser · 2014-08-26T23:37:35Z

It's still possible to create a field with a name and value, so no new api needed, right ?
Any kind of deprecation notice I add would catch normal usecases / be misleading since we cannot know if the "foo:" in a field is supposed to be the name or actual content (that's basically the bug...).
So maybe leave this hanging here until the project is ready for a bigger bump ?
Change should be pretty obvious to anyone using this kind of internal api (if anyone actually uses it...) since there will be duplicate fields everywhere...

jeremy · 2014-08-27T01:40:28Z

We could introduce a new API to instantiate a field with a value only, like Field.build value, or change #initialize to take either a string or value: ... argument.

The current Field.new "header: value" API may be deprecated and either removed or moved to a Field.parse ... API.

grosser · 2014-09-03T16:04:39Z

If someone needs the split feature they can just to Field.new *thing.split(":", 2), do you think it's a commonly used feature ?

grosser · 2014-09-03T16:05:13Z

and Field without name would just be Field.new nil, 'foo'

grosser · 2014-09-03T16:08:20Z

This is also not affecting basic Field usage, only subclasses, so ToField.new("To: abc") would have a different and obviously wrong result. Only usecase would be people reading in headers by themself, which seems a little weird ...

grosser · 2015-07-25T06:10:33Z

ran into this gain when doing outbound email ... would love to get this merged ... I don't see anyone using this api anyway ...

grosser · 2015-09-30T04:33:06Z

@jeremy rebased, can we get this merged now ?

(using this patch in apps that both handle inbound and outbound mail without any issues since over a year now ... )

kjg · 2015-09-30T14:55:16Z

@grosser You might have to rebase this again

grosser · 2015-09-30T15:49:07Z

done!

kjg · 2015-09-30T20:11:19Z

spec/mail/fields/common/common_field_spec.rb

Can we leave this spec in place?

feels useless to me ... but added it back if that makes you happy ...

Thanks! It's to prove that this bug is still fixed: https://github.com/mikel/mail/pull/719/files

@charset

Fixes mail with a legit "Subject: subject: foo" header. We'd split that into a subject field with a "subject: foo" value. When we instantiate the SubjectField and pass that value, we'd strip the leading "subject: " from it again! So we'd end up with a "foo" subject. This fixes the issue at the cost of some obscure backward compatibility. 1. If you instantiate internal header fields directly, bypassing Mail::Field, then using them to parse raw headers will now fail. This will now be treated as a literal value: Mail::ToField.new('To: foo@example.com') # => Mail::Field::IncompleteParseError: Mail::AddressList can not parse |To: foo@example.com|: Only able to parse up to "To: foo@example.com" Instead, use Mail::Field.parse: Mail::Field.parse('To: foo@example.com') # => #<Mail::ToField …> 2. If you parse fields by instantiating Mail::Field.new with a raw header, it'll now warn you with a deprecation error. That behavior will go away in Mail 2.8.0. Mail::Field.new('To: foo@example.com') # => Passing an unparsed header field to Mail::Field.new is deprecated and will be removed in Mail 2.8.0. Use Mail::Field.parse instead. # => #<Mail::Field 0x7fa401026910 @charset="utf-8" @name="To" @value="foo"> Instead, use Mail::Field.parse: Mail::Field.parse('To: foo@example.com') # => #<Mail::ToField …> Thanks to @grosser for #766 ❤️ Closes #1111

jeremy · 2017-05-22T11:02:18Z

#1111

seb-sykio · 2017-11-05T19:26:42Z

since I updated to 2.7.0, I have this issue :

  Failure/Error: mm = Dkim.sign(mm, :domain => xxx, :selector => 'mail', :private_key => OpenSSL::PKey::RSA.new(xxx))
  
  NoMethodError:
    undefined method `strip_field' for #<Mail::DkimField:0x0000564a55a59bf0 @charset="utf-8">
  # /xxx/.rvm/gems/ruby-2.4.2@xxx/gems/dkim-1.0.0/lib/mail/dkim_field.rb:12:in `initialize'

grosser mentioned this pull request Jul 31, 2014

Using Mail::Field.new can strip out content that looks like the field name #720

Merged

jeremy reviewed Aug 7, 2014
View reviewed changes

grosser mentioned this pull request May 14, 2015

Don't strip out content that looks like the field name from the middle of a field #719

Merged

grosser force-pushed the grosser/subject-double branch from 4ab36b5 to c0bfa2c Compare September 30, 2015 04:31

grosser force-pushed the grosser/subject-double branch from c0bfa2c to b7de62f Compare September 30, 2015 15:49

kjg reviewed Sep 30, 2015
View reviewed changes

stop stripping fields multiple times

24884b8

grosser force-pushed the grosser/subject-double branch from b7de62f to 24884b8 Compare September 30, 2015 23:36

jeremy mentioned this pull request May 22, 2017

Stop stripping header name prefixes from header values #1111

Closed

jeremy closed this May 22, 2017

jeremy added this to the 2.7.0 milestone May 22, 2017

seb-sykio mentioned this pull request Nov 6, 2017

undefined method `strip_field' after update to 2.7.0 #1169

Closed

mariozaizar deleted the grosser/subject-double branch July 26, 2018 22:48

remove duplicate field stripping #766

remove duplicate field stripping #766

Uh oh!

Conversation

grosser commented Jul 31, 2014

Uh oh!

jeremy commented Aug 1, 2014

Uh oh!

kjg commented Aug 1, 2014

Uh oh!

grosser commented Aug 1, 2014

Uh oh!

grosser commented Aug 1, 2014

Uh oh!

grosser commented Aug 6, 2014

Uh oh!

jeremy commented Aug 7, 2014

Uh oh!

jeremy Aug 7, 2014

Choose a reason for hiding this comment

Uh oh!

grosser commented Aug 8, 2014

Uh oh!

grosser commented Aug 12, 2014

Uh oh!

grosser commented Aug 16, 2014

Uh oh!

jeremy commented Aug 26, 2014

Uh oh!

grosser commented Aug 26, 2014

Uh oh!

jeremy commented Aug 27, 2014

Uh oh!

grosser commented Sep 3, 2014

Uh oh!

grosser commented Sep 3, 2014

Uh oh!

grosser commented Sep 3, 2014

Uh oh!

grosser commented Jul 25, 2015

Uh oh!

grosser commented Sep 30, 2015

Uh oh!

kjg commented Sep 30, 2015

Uh oh!

grosser commented Sep 30, 2015

Uh oh!

kjg Sep 30, 2015

Choose a reason for hiding this comment

Uh oh!

grosser Sep 30, 2015

Choose a reason for hiding this comment

Uh oh!

kjg Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

jeremy commented May 22, 2017

Uh oh!

seb-sykio commented Nov 5, 2017

Uh oh!

Uh oh!