Pass minification errors to the user

Previously, *minifyTransformation.Transform suppressed the
error returned by t.m.Minify. This meant that when minification
returned an error, the error would not reach the user. Instead,
minification would silently fail. For example, if a JavaScript
file included a call to the Date constructor with:

new Date(2020, 04, 02)

The package that the minification library uses to parse JS files,
github.com/tdewolff/parse would return an error, since "04" would
be parsed as a legacy octal. However, the JS file would remain
un-minified with no error.

Fixing this is not as simple as replacing "_" with an "err" in
*minifyTransformation.Transform, however (though this is
necessary). If we only returned this error from Transform,
then hugolib.TestResourceMinifyDisabled would fail. Instead of
being a no-op, as TestResourceMinifyDisabled expects, using the
"minify" template function with a "disableXML=true" config
setting instead returns the error, "minifier does not exist for
mimetype."

The "minifier does not exist" error is returned because of the
way minifiers.New works. If the user's config disables
minification for a particular MIME type, minifiers.New does
not add it to the resulting Client's *minify.M. However, this
also means that when the "minify" template function is executed,
 a *resourceAdapter's transformations still add a minification.
When it comes time to call the minify.Minifier for a specific
MIME type via *M.MinifyMimetype, the github.com/tdewolff/minify
library throws the "does not exist" error for the missing MIME
type.

The solution was to change minifiers.New so, instead of skipping
a minifier for each disabled MIME type, it adds  a NoOpMinifier,
which simply copies the source to the destination without
minification. This means that when the "minify" template
function is used for a particular resource, and that resource's
MIME type has minification disabled, minification is genuinely
skipped, and does not result in an error.

In order to add this, I've fixed a possibly unwanted interaction
between minifiers.TestConfigureMinify and
hugolib.TestResourceMinifyDisabled. The latter disables
minification and expects minification to be a no-op. The former
disables minification and expects it to result in an error. The
only reason hugolib.TestResourceMinifyDisabled passes in the
original code is that the "does not exist" error is suppressed.
However, we shouldn't suppress minification errors, since they
can leave users perplexed. I've changed the test assertion in
minifiers.TestConfigureMinify to expect no errors and a no-op
if minification is disabled for a particular MIME type.

Fixes #8954

diff --git a/hugolib/resource_chain_test.go b/hugolib/resource_chain_test.go
index d40d4c02edddb436b2343d168ee5f3b633677350..dee2f2146c0e6d36d93d3b619420d235687dd80a 100644 (file)
--- a/hugolib/resource_chain_test.go
+++ b/hugolib/resource_chain_test.go
@@ -1148,3 +1148,26 @@ XML: {{ $xml.Content | safeHTML }}|{{ $xml.RelPermalink }}
 XML: <root>   <foo> asdfasdf </foo> </root>|/xml/data.min.3be4fddd19aaebb18c48dd6645215b822df74701957d6d36e59f203f9c30fd9f.xml
 `)
 }
+
+// Issue 8954
+func TestMinifyWithError(t *testing.T) {
+       b := newTestSitesBuilder(t).WithSimpleConfigFile()
+       b.WithSourceFile(
+               "assets/js/test.js", `
+new Date(2002, 04, 11)
+`,
+       )
+       b.WithTemplates("index.html", `
+{{ $js := resources.Get "js/test.js" | minify | fingerprint }}
+<script>
+{{ $js.Content }}
+</script>
+`)
+       b.WithContent("page.md", "")
+
+       err := b.BuildE(BuildCfg{})
+
+       if err == nil || !strings.Contains(err.Error(), "04") {
+               t.Fatalf("expected a message about a legacy octal number, but got: %v", err)
+       }
+}

diff --git a/minifiers/minifiers.go b/minifiers/minifiers.go
index 3ac285ecbf8f6281dc310a8c9b5d436f1bd88e4b..5a5cec1217b196a833d090f91920a317d51982fd 100644 (file)
--- a/minifiers/minifiers.go
+++ b/minifiers/minifiers.go
@@ -57,6 +57,20 @@ func (m Client) Minify(mediatype media.Type, dst io.Writer, src io.Reader) error
        return m.m.Minify(mediatype.Type(), dst, src)
 }
 
+// noopMinifier implements minify.Minifier [1], but doesn't minify content. This means
+// that we can avoid missing minifiers for any MIME types in our minify.M, which
+// causes minify to return errors, while still allowing minification to be
+// disabled for specific types.
+//
+// [1]: https://pkg.go.dev/github.com/tdewolff/minify#Minifier
+type noopMinifier struct{}
+
+// Minify copies r into w without transformation.
+func (m noopMinifier) Minify(_ *minify.M, w io.Writer, r io.Reader, _ map[string]string) error {
+       _, err := io.Copy(w, r)
+       return err
+}
+
 // New creates a new Client with the provided MIME types as the mapping foundation.
 // The HTML minifier is also registered for additional HTML types (AMP etc.) in the
 // provided list of output formats.
@@ -69,47 +83,53 @@ func New(mediaTypes media.Types, outputFormats output.Formats, cfg config.Provid
        }
 
        // We use the Type definition of the media types defined in the site if found.
-       if !conf.DisableCSS {
-               addMinifier(m, mediaTypes, "css", &conf.Tdewolff.CSS)
-       }
-       if !conf.DisableJS {
-               addMinifier(m, mediaTypes, "js", &conf.Tdewolff.JS)
-               m.AddRegexp(regexp.MustCompile("^(application|text)/(x-)?(java|ecma)script$"), &conf.Tdewolff.JS)
-       }
-       if !conf.DisableJSON {
-               addMinifier(m, mediaTypes, "json", &conf.Tdewolff.JSON)
-               m.AddRegexp(regexp.MustCompile(`^(application|text)/(x-|(ld|manifest)\+)?json$`), &conf.Tdewolff.JSON)
-       }
-       if !conf.DisableSVG {
-               addMinifier(m, mediaTypes, "svg", &conf.Tdewolff.SVG)
-       }
-       if !conf.DisableXML {
-               addMinifier(m, mediaTypes, "xml", &conf.Tdewolff.XML)
-       }
+       addMinifier(m, mediaTypes, "css", getMinifier(conf, "css"))
+
+       addMinifier(m, mediaTypes, "js", getMinifier(conf, "js"))
+       m.AddRegexp(regexp.MustCompile("^(application|text)/(x-)?(java|ecma)script$"), getMinifier(conf, "js"))
+
+       addMinifier(m, mediaTypes, "json", getMinifier(conf, "json"))
+       m.AddRegexp(regexp.MustCompile(`^(application|text)/(x-|(ld|manifest)\+)?json$`), getMinifier(conf, "json"))
+
+       addMinifier(m, mediaTypes, "svg", getMinifier(conf, "svg"))
+
+       addMinifier(m, mediaTypes, "xml", getMinifier(conf, "xml"))
 
        // HTML
-       if !conf.DisableHTML {
-               addMinifier(m, mediaTypes, "html", &conf.Tdewolff.HTML)
-               for _, of := range outputFormats {
-                       if of.IsHTML {
-                               m.Add(of.MediaType.Type(), &conf.Tdewolff.HTML)
-                       }
+       addMinifier(m, mediaTypes, "html", getMinifier(conf, "html"))
+       for _, of := range outputFormats {
+               if of.IsHTML {
+                       m.Add(of.MediaType.Type(), getMinifier(conf, "html"))
                }
        }
 
        return Client{m: m, MinifyOutput: conf.MinifyOutput}, nil
 }
 
-func addMinifier(m *minify.M, mt media.Types, suffix string, min minify.Minifier) {
-       types := mt.BySuffix(suffix)
-       for _, t := range types {
-               m.Add(t.Type(), min)
+// getMinifier returns the appropriate minify.MinifierFunc for the MIME
+// type suffix s, given the config c.
+func getMinifier(c minifyConfig, s string) minify.Minifier {
+       switch {
+       case s == "css" && !c.DisableCSS:
+               return &c.Tdewolff.CSS
+       case s == "js" && !c.DisableJS:
+               return &c.Tdewolff.JS
+       case s == "json" && !c.DisableJSON:
+               return &c.Tdewolff.JSON
+       case s == "svg" && !c.DisableSVG:
+               return &c.Tdewolff.SVG
+       case s == "xml" && !c.DisableXML:
+               return &c.Tdewolff.XML
+       case s == "html" && !c.DisableHTML:
+               return &c.Tdewolff.HTML
+       default:
+               return noopMinifier{}
        }
 }
 
-func addMinifierFunc(m *minify.M, mt media.Types, suffix string, min minify.MinifierFunc) {
+func addMinifier(m *minify.M, mt media.Types, suffix string, min minify.Minifier) {
        types := mt.BySuffix(suffix)
        for _, t := range types {
-               m.AddFunc(t.Type(), min)
+               m.Add(t.Type(), min)
        }
 }

diff --git a/minifiers/minifiers_test.go b/minifiers/minifiers_test.go
index 81edba5103e2910a61e6e1b6a11c8a7c8f834cd1..ece8cbd08cf032b9699d43a8089ac91a65de34bf 100644 (file)
--- a/minifiers/minifiers_test.go
+++ b/minifiers/minifiers_test.go
@@ -93,9 +93,9 @@ func TestConfigureMinify(t *testing.T) {
                expectedMinString string
                errorExpected     bool
        }{
-               {media.HTMLType, "<hello> Hugo! </hello>", "<hello> Hugo! </hello>", false}, // configured minifier
-               {media.CSSType, " body { color: blue; }  ", "body{color:blue}", false},      // default minifier
-               {media.XMLType, " <hello>  Hugo!   </hello>  ", "", true},                   // disable Xml minification
+               {media.HTMLType, "<hello> Hugo! </hello>", "<hello> Hugo! </hello>", false},            // configured minifier
+               {media.CSSType, " body { color: blue; }  ", "body{color:blue}", false},                 // default minifier
+               {media.XMLType, " <hello>  Hugo!   </hello>  ", " <hello>  Hugo!   </hello>  ", false}, // disable Xml minification
        } {
                var b bytes.Buffer
                if !test.errorExpected {

diff --git a/resources/resource_transformers/minifier/minify.go b/resources/resource_transformers/minifier/minify.go
index 972461e0e741f14eef3fd2a9088cdd49d81a7143..c00d478af7e10587dcae5ccb24f4f5637cb1e925 100644 (file)
--- a/resources/resource_transformers/minifier/minify.go
+++ b/resources/resource_transformers/minifier/minify.go
@@ -47,9 +47,8 @@ func (t *minifyTransformation) Key() internal.ResourceTransformationKey {
 }
 
 func (t *minifyTransformation) Transform(ctx *resources.ResourceTransformationCtx) error {
-       _ = t.m.Minify(ctx.InMediaType, ctx.To, ctx.From)
        ctx.AddOutPathIdentifier(".min")
-       return nil
+       return t.m.Minify(ctx.InMediaType, ctx.To, ctx.From)
 }
 
 func (c *Client) Minify(res resources.ResourceTransformer) (resource.Resource, error) {