Change in error raising behaviour about mjml-rails

[sighmon]

@denny How does version 4.5.0 look to you? Does that work for your workflow?

[denny]

Here's a minimal change (just the condition on line 40) that makes my test pass... but it doesn't seem likely to be the best solution:

denny@rocinante:~/ShinyCMS$ rspec ./plugins/ShinyNewsletters/spec/models/shiny_newsletters/template_spec.rb:46
ShinyNewsletters::Template
  validations
    mjml_syntax

From: /home/denny/.rvm/gems/ruby-2.7.2/gems/mjml-rails-4.4.1/lib/mjml/parser.rb:40 Mjml::Parser#run:
    38:     command = "-r #{in_tmp_file} -o #{out_tmp_file.path} --config.beautify #{beautify} --config.minify #{minify} --config.validationLevel #{validation_level}"
    39:     _, stderr, status = Mjml.run_mjml(command)
 => 40:     binding.pry
    41:     raise ParseError.new(stderr.chomp) unless stderr.blank? # status.success?

[1] pry(#<Mjml::Parser>)> status.success?
true
[2] pry(#<Mjml::Parser>)> stderr.blank?
false
[3] pry(#<Mjml::Parser>)> stderr
"Line 4 of /tmp/in20201126-2469595-nhp775.mjml (mj-title) — mj-title cannot be used inside mj-column, only inside: mj-attributes, mj-head\n"
[4] pry(#<Mjml::Parser>)> 
      fails to create a new Template if the template file is not valid MJML

Finished in 32.81 seconds (files took 3.1 seconds to load)
1 example, 0 failures

[sighmon]

@doits status.success? seems to be returning true for MJML template syntax errors.

I'll push the fix @denny mentions checking for stderr.blank? (thanks for the thorough bug report).

@doits Was there a reason you chose Open3.capture3 over Open3.popen3 and the change in error checking to status.success??

[doits]

Was there a reason you chose Open3.capture3 over Open3.popen3 and the change in error checking to status.success??

Actually I was under the impression that mjml will return a correct error code when compilation fails, but I didn't check it – sorry for breaking it! I created #71 with a test for it so it won't break next time.

[doits]

I took a dive again and made another PR #72 with some more explanation.

[chasegiunta]

@sighmon @doits @denny So, the change between 4.4.1 & 4.4.2 (and subsequent 4.5.0 & 4.6.0 releases) has caused templates with mjml errors to not render at all and fail silently if:

Mjml.setup do |config|
   config.raise_render_exception = false
end

Previously, in 4.4.1, you could still render a template with a validation error, and suppress the exception(s) with the config above. With 4.4.2, while errors are apparent if config.raise_render_exception = true, absolutely nothing will render if that setting is set to false, and no errors will be output.

Been driving me mad for the past day trying to figure this out, so let me know if there's anything I can do to help fix this. Otherwise, the config.raise_render_exception = false is not applicable currently.

[doits]

Yes, to solve your issue only set config.validation_level = "soft" if you want to render templates with errors. This tells MJML-Rails to not raise an exception if only the template is invalid.

Normally you do not want to render malformed templates, that is why it raises an exception by default. You shouldn't change config.raise_render_exception for your use case. With validation level soft exceptions are only raised if something really bad happens, not if the template is malformed, and therefore you shouldn't ignore exceptions if you depend on MJML – because config.raise_render_exception = false is exactly the setting to tell MJML-Rails to fail silently.

I'm not sure though that it was possible to render invalid templates in version 4.4.0 at all, because MJML-Rails checked standard error output to decide whether to raise an exception or not. So if there was any error in the template it should have raised an exception there and ignoring it always returned an empty string from my understanding.

Maybe you did update from a version before 4.4.0 (4.4.0 added validation level) or update MJML at the same time?

[doits]

Actually I think you are right – only in version 4.4.1 it did render invalid templates, which was a bug though (and it had nothing to do with raise_render_exception = false). 4.4.1 was released only for a short time (I think a day?) and I'm sorry if you hit this and based your assumptions/config on this.

[chasegiunta]

@doits That makes sense. Thanks for the thorough explanation 👍