Closed denny closed 3 years ago
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
@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?
?
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.
I took a dive again and made another PR #72 with some more explanation.
@denny How does version 4.5.0
look to you? Does that work for your workflow?
@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.
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?
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.
@doits That makes sense. Thanks for the thorough explanation 👍
Hi,
I'm seeing a change in error-raising behaviour between 4.4.0 and 4.4.1, and looking at the code I think it may be a bug in the Parser.run method - specifically where it decides whether to raise an exception for an invalid MJML input file: https://github.com/sighmon/mjml-rails/commit/063152ce6dbc7ae578063d14dde466199d9325d0#diff-5c149cf160539fd426925ca3b65fc5dafb2a1c9d5ae56df3212543e112d242fdR40
In every case that I've tested (three or four, not loads),
status.success?
is returning true when the MJML template is invalid, wherestderr
contains content andstderr.eof?
returned false with the previous code. This means that instead of a ParseError being raised, an empty string is always rendered and returned - regardless of the setting of Mjml.raise_render_exception.Previously I was catching the ParseError in a validator in my Rails app when people attached an MJML template to a database record - so with 4.4.1, my validator passes everything and my CMS allows items to be saved with invalid syntax in their related MJML files.
I'll post some more info below, let me know if you need something specific that I haven't included. Also if there's a better way I should be checking for valid MJML syntax in an ActiveRecord validator :)
Cheers, Denny
(I've edited the above runs for length, mostly by removing the first pass each takes through the MJML parser, which I assume is the layout.)