search

mjmlio / mjml

MJML: the only framework that makes responsive-email easy
https://mjml.io
MIT License
17.08k stars 960 forks source link

Message in stderr is breaking django-mjml #1439

Closed rosacris closed 5 years ago

rosacris commented 5 years ago

The fix to issue #1342 released in version 4.2.1 is breaking django-mjml template engine. If using mjml v3 the message MJML v3 syntax detected, migrating to MJML v4 syntax. Use mjml -m to get the migrated MJML. is shown in stderr now, making django-mjml fail to render the template because it considers that any output in stderr an error running the command. With mjml v4.2.0 it works just fine.

iRyusa commented 5 years ago

Well as the migration won't be available forever in the mjml client I think it should still throw in the stderr ?

On Mon, Nov 26, 2018 at 10:46 PM Cristian Rosa notifications@github.com wrote:

The fix to issue #1342 https://github.com/mjmlio/mjml/issues/1342 released in version 4.2.1 is breaking django-mjml template engine. If using mjml v3 the message MJML v3 syntax detected, migrating to MJML v4 syntax. Use mjml -m to get the migrated MJML. is shown in stderr now, making django-mjml fail to render the template because it considers that any output in stderr an error running the command. With mjml v4.2.0 it works just fine.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mjmlio/mjml/issues/1439, or mute the thread https://github.com/notifications/unsubscribe-auth/AAizzWO4wmlSzHK7LEAiomJyHhZYoDp7ks5uzGE3gaJpZM4Yz46T .

-- Cordialement, Maxime BRAZEILLES

rosacris commented 5 years ago

I agree, I will fix it by migrating the templates to v4. I just wanted to point it out that depending on what do you consider the API of mjml, 4.2.1 was a minor relase that changed the API (moving stuff from stdout to stderr). Anyways, I will create an issue in django-mjml as well. Thanks!

iRyusa commented 5 years ago

Yup totally agree that should have been merged in 4.3 at least... my bad on that !

iRyusa commented 5 years ago

Well as we can't do really something about this, i'm closing the ticket.

ngarnier commented 5 years ago

Hello, we are going to reopen this issue and fix the behaviour by sending MJML validation errors to stdout instead. stderr should be used only for an actual failure in the MJML compilation, not syntax warnings.

iRyusa commented 5 years ago

What about stderr for strict & stdout for other level ?

tkafka commented 5 years ago

Hi @ngarnier, I disagree with this - stdout should only be used for the actual output (so that you can redirect it to a file, and be sure you aren't sending your users an email with validation messages).

All diagnostic output should go to stderr, and I propose to add setting to select verbosity (warnings and errors, or errors only).

liminspace commented 5 years ago

@iRyusa @ngarnier I think stdout must give us the result; any errors, warnings, etc. must be given us from stderr and verbosity option must filter the messages. It is simple and clearly. I can't understand why changing standards everyday is normal practice in js-world.

ngarnier commented 5 years ago

Ok, I thought it was a problem for you to have the errors in stderr but as you're unhappy with both my proposal and @iRyusa, I guess I misunderstood. We're going to leave it as is.

ayadcodes commented 3 years ago

I had this problem recently, the solution was to remove mj-container as mentioned here #1342 by @iRyusa