instance_variable_get issue on mail 2.5.4
Closed this issue · 31 comments
Hi,
sorry it took some time to figure out what was the real issue what I mentioned. At last I caught it when I tested this new things under our webpage. I didn't realise it when I downloaded the new codebase that the mail gem was updated and that's why I couldn't see the issue anymore. So here is some debugging:
mail 2.5.4
Run options: include {:locations=>{"./spec/mandrill_dm/message_spec.rb"=>[120]}}
>>>>>>>>>>>>>>mail: #<Mail::Message:70360916361720, Multipart: false, Headers: <global-merge-vars: {"name"=>"TESTVAR", "content"=>"testcontent"}, {"name"=>"TESTVAR2", "content"=>"testcontent2"}>>
>>>>>>>>>>>>>>mail[field]: #<Mail::Field:0x007ffc5a1987b0 @field=#<Mail::OptionalField:0x007ffc5a15a438 @errors=[], @charset=#<Encoding:UTF-8>, @name="global-merge-vars", @length=nil, @tree=nil, @element=nil, @value="{\"name\"=>\"TESTVAR\", \"content\"=>\"testcontent\"}, {\"name\"=>\"TESTVAR2\", \"content\"=>\"testcontent2\"}">>
>>>>>>>>>>>>>>mail[field].instance_variable_get('@value'): nil
F
Failures:
1) MandrillDm::Message#global_merge_vars takes an array of multiple global_merge_vars
Failure/Error: expect(message.global_merge_vars).to eq(global_merge_vars)
expected: [{"name"=>"TESTVAR", "content"=>"testcontent"}, {"name"=>"TESTVAR2", "content"=>"testcontent2"}]
got: nil
(compared using ==)
# ./spec/mandrill_dm/message_spec.rb:127:in `block (3 levels) in <top (required)>'
Finished in 0.01995 seconds (files took 0.32437 seconds to load)
1 example, 1 failure
mail 2.6.3
Run options: include {:locations=>{"./spec/mandrill_dm/message_spec.rb"=>[120]}}
>>>>>>>>>>>>>>mail: #<Mail::Message:70212615828160, Multipart: false, Headers: <global-merge-vars: {"name"=>"TESTVAR", "content"=>"testcontent"}, {"name"=>"TESTVAR2", "content"=>"testcontent2"}>>
>>>>>>>>>>>>>>mail[field]: #<Mail::Field 0x7fb74c0dfdf8 @name="global-merge-vars" @value=[{"name"=>"TESTVAR", "content"=>"testcontent"}, {"name"=>"TESTVAR2", "content"=>"testcontent2"}] @raw_value=nil @charset=nil @field=#<Mail::OptionalField:0x007fb74c0de6d8 @errors=[], @charset=#<Encoding:UTF-8>, @name="global-merge-vars", @length=nil, @tree=nil, @element=nil, @value="{\"name\"=>\"TESTVAR\", \"content\"=>\"testcontent\"}, {\"name\"=>\"TESTVAR2\", \"content\"=>\"testcontent2\"}">>
>>>>>>>>>>>>>>mail[field].instance_variable_get('@value'): [{"name"=>"TESTVAR", "content"=>"testcontent"}, {"name"=>"TESTVAR2", "content"=>"testcontent2"}]
.
Finished in 0.00257 seconds (files took 0.24934 seconds to load)
1 example, 0 failures
So on mail 2.5.4 the [] is missing from the @vaule what is cause the problem. Or actually here the value just a json like version, but on the new mail you get the normal value and a json like one as well.
The real problem is that the actionmailer still using mail 2.5 (Rails 4.2.5 >> mail >= 2.5.4, ~> 2.5). So this issue is affected in all Rails version!
@Sysqa I am on Rails 4.2.5 and on mail 2.6.3. Still, this means our code does not work on any mail version.
@spovich what's your opinion? Should we lock min version of mail in gemspec or try to look for alternative solution for getting the variables out of the mailer?
hmm... I don't think there is an elegant way to restore the string'ified hash back to a ruby hash.
I don't mind specifying a minimum mail gem version. Are there any restrictions on mail >= 2.6.x on older rails?
Having said that, I'm not really happy with the current way we are getting @value. I think we should be using the interface which is mail[:foo].value, so I think we are left with JSON.parse.
@spovich as I seen on Rails 3.2.22 I can't update the mail version to 2.6, but would be great if somebody else could try it as well.
@value was not available before 2.6 and there's no safe way to parse a result of inspect, which is returned by mail[:foo].value.
Rails 4 uses 2.6 (~> 2.5 means 2.*). IMO we should drop Rails 3 support and use .instance_variable_get('@value'). @spovich what do you think?
@kshnurov I think it is too soon to drop support rails 3.2.
From the output @Sysqa has above, he is showing @value for 2.5 and 2.6. However, in 2.5, @value is a string and has lost the []. Perhaps we can check if @value is a string, if yes, use the json parse, otherwise take the resulting value (so it works for both)?
@spovich I've looked inside Mail::Field - there was no @value in 2.5, this is why @Sysqa is getting nil's.
Once again - JSON parse won't give you the right results, you will get parse exceptions and data corruption all the time. The only way to revert inspect is eval, which is absolutely not an option.
Everything that's passed to mail() or directly to headers[] will be stored and returned as a string. Since 2.6 original value is stored in @value and we can get it. Before 2.6 - there's no way.
The only option for 2.5 is to extend ActionMailer::Base with merge_vars and other methods, so you can call them like this:
def welcome(recipient)
headers['X-Vary'] = 'whatever'
merge_vars << {...}
mail(...)
end
I don't think it's worth it, 3.2 is more than 2.5 years old and 5.0 is almost out.
Hi just add some thoughts here.
I think until Rails 3.2 is a maintanance version this gem should support the older Rails as well. Because there will be many people who would like to use this and stuck on a older Rails.
@kshnurov I know the JSON parse is not the best solution. But... How can we solve that problem? I will really happy if you provide some ideas, because I need to solve this problem and any better solutions are welcome.
@Sysqa I've described one possible solution 2 comments above. #31 (comment)
One more solution is to monkey-patch Mail::OptionalField initializer for 2.5 to make instance_variable_get('@value') work. It's a dirty way.
module Mail
class OptionalField < UnstructuredField
def initialize(name, value, charset = nil)
@value = value unless defined?(@value)
super(name, value, charset)
end
end
end
People who stuck on Rails 3 should either upgrade or prepare to suffer, cause many gems already dropped R3 support. Supporting R3 will take more effort and more ugly code like this. Those who wants to stay can always use an older version of this gem.
@kshnurov how about Ruby refinements (http://yehudakatz.com/2010/11/30/ruby-2-0-refinements-in-practice/)? That's Ruby 2+, but a clean way to add the @value.
@kshnurov then I'd propose detecting the version of Mail and
- either monkey patching it
- or dynamically extending the gem with the merge_vars/global_vars support when possible.
?
With Mail 2.5 merge_vars/global_merge_vars just silently don't work, they pass nil's to Mandrill. I think it would be ok to add an instruction for 2.5 users on how to monkey-patch their project. Including monkey-patch into a gem is a very bad idea, you never know how it ends.
How about we display warning message when merge_vars / global_vars are being passed, yet we're not able to retrieve the @value? I don't like the idea of silently not working – could drive someone mad (been there ;-)).
no silent things +1
Ok, @Sysqa , @tomasc , could you, please, confirm that monkey-patch solution works with 2.5? I'll then add an exception
If the monkey patch works, I'm good with this approach. To be clear, as @kshnurov said, we definitely don't want our code to be doing any monkey-patching; that's entirely up to the end users. We should reference the wiki monkey-patch in the README for 2.5.x users.
I like the idea of raising an exception in get_value, so that covers any use of it. Would need to check for the monkey-patch and not raise if it responds_to? @value.
@Sysqa did you require 'mail' before the monkey-patch? Rails lazy-loads, so mail needs to be force loaded to apply the patch. Also, I assume you put your monkey-patch into a file in config/initializers/foo.rb?
@spovich you were right I missed the require 'mail' but still not working. So if I have time I'll try to debug how it's working.
@spovich @kshnurov This is what worked for me:
require 'mail'
module MailFieldMonkeyPatch
def initialize(name, value = nil, charset = 'utf-8')
@value = value unless defined?(@value)
super(name, value, charset)
end
end
module Mail
class Field
prepend MailFieldMonkeyPatch
end
endI don't know there will be any side affect about this modification but at this moment it looks ok.
@Sysqa Nice! Glad you got this working!
Small note: you can clean this up a little bit by calling super without any arguments. When you do this, ruby forwards the original arguments to the super method. Typically, you only include arguments to super if you are changing the method signature or mutating an argument. Also, note that super is different than super(). The latter passes no arguments.
@spovich yes you're right, so I think we have a working monkey patch and we could step forward.
require 'mail'
module MailFieldMonkeyPatch
def initialize(name, value = nil, charset = 'utf-8')
@value = value unless defined?(@value)
super
end
end
module Mail
class Field
prepend MailFieldMonkeyPatch
end
end@Sysqa this is good for ruby 2.0+ Can you update the wiki page? https://github.com/spovich/mandrill_dm/wiki/Rails-3-(Mail-2.5)-support
Ruby 1.9 users would need to use alias_method to get prepend-like functionality.
@spovich sure I will update it...
@Sysqa @spovich you should monkey-patch only OptionalField, not all Field's. I've updated the wiki page, please check if it works.
@kshnurov did you try it? I ask it because it didn't work for me. Neither OptionalField nor UnstructuredField. So that's why I used Field
@Sysqa ok, let's monkey-patch Field, mail 2.6 have @value in all Field's. I've updated the wiki, this issue could be closed.