Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mjml --validate not working properly #1356

Closed
gabiudrescu opened this issue Sep 24, 2018 · 11 comments
Closed

mjml --validate not working properly #1356

gabiudrescu opened this issue Sep 24, 2018 · 11 comments

Comments

@gabiudrescu
Copy link

gabiudrescu commented Sep 24, 2018

I'm trying to setup a repository for our newsletters with a build system in place that ensures no developer is committing malformed MJML code.

This build system should run mjml --validate files/* and fail if any of the files in that repo have typos or illegal attributes.

As I have mentioned in several comments in #198 this is not happening. I guess I found a way to reproduce this, somehow, consistently.

Reproduction Steps:

I have the file header.mjml with the following code:


<mj-section padding="6px 15px" background-color="#E5E5E5">
  <mj-column>
    <mj-text mj-class="online-version"><a class="online-version-link" href="[#online]">VIEW ONLINE VERSION</a>
    </mj-text>
  </mj-column>
</mj-section>

<mj-section padding="0">
  <mj-column padding-top="30px">
    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" />
  </mj-column>
</mj-section>

<mj-section background-color="#ffffff">
  <mj-column padding-top="16px">
    <mj-navbar base-url="https://mjml.io" hamburger="hamburger" ico-color="#141313">
        <mj-navbar-link href="#" color="#141313" font-size="12px">WHAT'S NEW</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">FRAGRANCES</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">MAKE-UP</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">SKINCARE</mj-navbar-link>
        <mj-navbar-link href="#" color="#f50e34" font-size="12px">DISCOVER MORE</mj-navbar-link>
  </mj-column>
</mj-section>

If I change the mjml-image this to:

<mj-section padding="0">
  <mj-column padding-top="30px">
-    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" />
+    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" invalid="blabla" />
  </mj-column>
</mj-section>

and I run the command node_modules/.bin/mjml --validate mjml/includes/header.mjml I get no output in the console.

same behavior if I change the code to the following:

<mj-section padding="0">
  <mj-column padding-top="30px">
+    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" illegal="attribute" />
    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" />
  </mj-column>
</mj-section>

the strange thing is that if I run the same command over the file having only the content below:

<mj-section padding="0">
    <mj-column padding-top="30px">
        <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" illegal="attribute" />
        <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" />
    </mj-column>
</mj-section>

I get the expected error in console:

root@287f03a591c3:/data# node_modules/.bin/mjml --validate mjml/includes/header.mjml 
Line 3 of /data (mj-image) — Attribute illegal is illegal

Command line error:
Validation failed

Expected behavior:

node_modules/.bin/mjml --validate mjml/includes/header.mjml should always fail when I have the following code in the file:

<mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" illegal="attribute" />

Observed behavior:

Sometimes, the wrong code does not trigger a validation error.

MJML version:

root@287f03a591c3:/data# npm list mjml
npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
[email protected] /data
`-- [email protected]
  `-- [email protected] 

npm info ok 
root@287f03a591c3:/data# 

Email clients the bug is seen on:

N/A

@iRyusa
Copy link
Member

iRyusa commented Sep 24, 2018

Wait, so header.mjml is just an include right ? With no mj-head & body ?

@gabiudrescu
Copy link
Author

yes.

@iRyusa
Copy link
Member

iRyusa commented Sep 24, 2018

So that's why we can't really validate a include without the context of where it's included. The validator already validate nested include so that's the reason why it doesn't output anything.

Edit: note that include can be described as a full MJML document. Main issue would be if it doesn't start with a mj-section it will trigger an error on validation whereas it can be totally used in a document

@gabiudrescu
Copy link
Author

gabiudrescu commented Sep 24, 2018

I'm not sure I totally understand your last post, but if I try to run the validator against index.mjml which includes the file includes/header.mjml I still don't get an error if it contains <mj-image (...) illegal="attribute" />

although, it should.

Steps to reproduce:
Create a file index.mjml with the following content:

<mjml>
  <mj-head>
    <mj-title>BestValue Email Components</mj-title>
  </mj-head>

  <mj-body background-color="#ffffff" color="#141313">
    <mj-include path="includes/header.mjml" />
  </mj-body>
</mjml>

Create another file includes/header.mjml with the following invalid content:


<mj-section padding="6px 15px" background-color="#E5E5E5">
  <mj-column>
    <mj-text mj-class="online-version"><a class="online-version-link" href="[#online]">VIEW ONLINE VERSION</a>
    </mj-text>
  </mj-column>
</mj-section>

<mj-section padding="0">
  <mj-column padding-top="30px">
    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" illegal="attribute" />
  </mj-column>
</mj-section>

<mj-section background-color="#ffffff">
  <mj-column padding-top="16px">
    <mj-navbar base-url="https://mjml.io" hamburger="hamburger" ico-color="#141313">
        <mj-navbar-link href="#" color="#141313" font-size="12px">WHAT'S NEW</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">FRAGRANCES</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">MAKE-UP</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">SKINCARE</mj-navbar-link>
        <mj-navbar-link href="#" color="#f50e34" font-size="12px">DISCOVER MORE</mj-navbar-link>
  </mj-column>
</mj-section>

there are two errors in this file:

  • the illegal="attribute"
  • the missing closing tag for `

Both commands below return no error:

root@2adc28d3172b:/data# node_modules/.bin/mjml --validate mjml/includes/header.mjml
root@2adc28d3172b:/data# node_modules/.bin/mjml --validate mjml/index.mjml

If I modify mjml/includes/header.mjml to have wrapping mjml and/or mj-body, I start to receive output when running root@2adc28d3172b:/data# node_modules/.bin/mjml --validate mjml/includes/header.mjml:

Content header.mjml:

<mjml>
    <mj-body>
        <mj-section padding="6px 15px" background-color="#E5E5E5">
            <mj-column>
                <mj-text mj-class="online-version"><a class="online-version-link" href="[#online]">VIEW ONLINE VERSION</a>
                </mj-text>
            </mj-column>
        </mj-section>

        <mj-section padding="0">
            <mj-column padding-top="30px">
                <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" illegal="attribute" />
            </mj-column>
        </mj-section>

        <mj-section background-color="#ffffff">
            <mj-column padding-top="16px">
                <mj-navbar base-url="https://mjml.io" hamburger="hamburger" ico-color="#141313">
                    <mj-navbar-link href="#" color="#141313" font-size="12px">WHAT'S NEW</mj-navbar-link>
                    <mj-navbar-link href="#" color="#141313" font-size="12px">FRAGRANCES</mj-navbar-link>
                    <mj-navbar-link href="#" color="#141313" font-size="12px">MAKE-UP</mj-navbar-link>
                    <mj-navbar-link href="#" color="#141313" font-size="12px">SKINCARE</mj-navbar-link>
                    <mj-navbar-link href="#" color="#f50e34" font-size="12px">DISCOVER MORE</mj-navbar-link>
            </mj-column>
        </mj-section>
    </mj-body>
</mjml>

Output:

root@2adc28d3172b:/data# node_modules/.bin/mjml --validate mjml/includes/header.mjml
Line 12 of /data (mj-image) — Attribute illegal is illegal

Command line error:
Validation failed

But still no output for root@2adc28d3172b:/data# node_modules/.bin/mjml --validate mjml/index.mjml.

What's even worse (for me) is that I get no error on the unclosed MJML tag.

@iRyusa
Copy link
Member

iRyusa commented Sep 24, 2018

➜  mjml-4-1 vim included.mjml
➜  mjml-4-1 vim main.mjml
➜  mjml-4-1 yarn mjml --validate main.mjml
yarn run v1.7.0
$ /Users/maximebrazeilles/mjml-4-1/node_modules/.bin/mjml --validate main.mjml
Line 10 of /Users/maximebrazeilles/mjml-4-1/included.mjml, included at line 7 of file /Users/maximebrazeilles/mjml-4-1 (mj-image) — Attribute illegal is illegal

Command line error:
Validation failed
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
➜  mjml-4-1 cat main.mjml
<mjml>
  <mj-head>
    <mj-title>BestValue Email Components</mj-title>
  </mj-head>

  <mj-body background-color="#ffffff" color="#141313">
    <mj-include path="included.mjml" />
  </mj-body>
</mjml>
➜  mjml-4-1 cat included.mjml
<mj-section padding="6px 15px" background-color="#E5E5E5">
  <mj-column>
    <mj-text mj-class="online-version"><a class="online-version-link" href="[#online]">VIEW ONLINE VERSION</a>
    </mj-text>
  </mj-column>
</mj-section>

<mj-section padding="0">
  <mj-column padding-top="30px">
    <mj-image width="203px" src="images/logo.png" align="center" padding="0"  href="https://bestvalue.eu" target="_blank" illegal="attribute" />
  </mj-column>
</mj-section>

<mj-section background-color="#ffffff">
  <mj-column padding-top="16px">
    <mj-navbar base-url="https://mjml.io" hamburger="hamburger" ico-color="#141313">
        <mj-navbar-link href="#" color="#141313" font-size="12px">WHAT'S NEW</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">FRAGRANCES</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">MAKE-UP</mj-navbar-link>
        <mj-navbar-link href="#" color="#141313" font-size="12px">SKINCARE</mj-navbar-link>
        <mj-navbar-link href="#" color="#f50e34" font-size="12px">DISCOVER MORE</mj-navbar-link>
  </mj-column>
</mj-section>
➜  mjml-4-1

I'm almost sure either you have a conflict issue with an old MJML version or somewhat, it works just fine

@iRyusa
Copy link
Member

iRyusa commented Sep 24, 2018

About the unclosed tag, there's no way to identify those for now with the current parser implem.

And I'm not sure that we would be able to identify it anyway, because you're closing the parent tag (mj-column) here.

@gabiudrescu
Copy link
Author

The current parser here is raising a warning (not an error): https://mjml.io/try-it-live/BJuqJtIFQ

@iRyusa
Copy link
Member

iRyusa commented Sep 24, 2018

Well the issue is on the try it live we're using a basic XML parser from CodeMirror that just tells you when an XML document isn't valid so it's not really related to MJML.
That's not what we're using for --validate, so basically let's not compare the try it live to the proper linter.

@gabiudrescu
Copy link
Author

between your implementation above and mine there are two differences:

  • you are using yarn instead of npm
  • you have both files under the same folder, I have the include in another folder

I'll give it a try as you suggested, to see if it happens. can you also share with me your environment setup? npm version, yarn version etc. ? or, preferably, a docker file I can use?

@iRyusa
Copy link
Member

iRyusa commented Sep 25, 2018

yarn/npm doesn't mean much here, yarn just allow a shortcut to run binary, would be the same to you node_modules/.bin/mjml ...

➜  mjml-4-1 node_modules/.bin/mjml --validate main.mjml
Line 10 of /Users/maximebrazeilles/mjml-4-1/subfolder/included.mjml, included at line 7 of file /Users/maximebrazeilles/mjml-4-1 (mj-image) — Attribute illegal is illegal
Line 27 of /Users/maximebrazeilles/mjml-4-1/subfolder/included.mjml, included at line 7 of file /Users/maximebrazeilles/mjml-4-1 (mj-image) — Attribute illegal is illegal

Command line error:
Validation failed

And it does exactly the same here with subfolder.

➜  mjml-4-1 yarn mjml main.mjml
yarn run v1.7.0
$ /Users/maximebrazeilles/mjml-4-1/node_modules/.bin/mjml main.mjml
Line 10 of /Users/maximebrazeilles/mjml-4-1/subfolder/included.mjml, included at line 7 of file /Users/maximebrazeilles/mjml-4-1/main.mjml (mj-image) — Attribute illegal is illegal
Line 27 of /Users/maximebrazeilles/mjml-4-1/subfolder/included.mjml, included at line 7 of file /Users/maximebrazeilles/mjml-4-1/main.mjml (mj-image) — Attribute illegal is illegal

I'm using node 9.5, but don't see any difference from TLS, to this one.

Do you install MJML globally ? If yes, please clean it, remove every mjml-related package and do a full reinstall.

Are you on Windows ? Does the included file is well replaced on build ? If you don't use the cli to build it, do you modify the filePath option ?

@iRyusa
Copy link
Member

iRyusa commented Oct 2, 2018

Well, as mentioned, it works just fine for us, non-closed tags are really hard to identify. I wonder if parser can detect if a tag is properly closed (cc @kmcb777, could be a new rule to add for 4.3 ?)

It looks most likely like an env issue from you. Feel free to reopen if you have more information to share

@iRyusa iRyusa closed this as completed Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants