Repository: mmcdole/gofeed
Pull request #148 (2020-05-28 13:11:29)

Allow unknown html entities
I've encounter few feeds with html entities in <title> or <description> for ex.
`<title>Site d&#8217;actualit&eacute; g&eacute;n&eacute;raliste</title>`

Instead of clean UTF8:
`<title>Site d’actualité généraliste</title>`

Even if it's not a valid XML, it's quite easy to be tolerant, and not reject the feed.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/31090200/badge)](https://coveralls.io/builds/31090200)

Coverage increased (+0.08%) to 60.891% when pulling **b3e604c50e932936933fe8123fde8738002bca45 on florentsolt:master** into **4298e4366be3f64365cf257806159ed24bf89a4f on mmcdole:master**.

Necoro: Instead of adding html.UnescapeString only as the default case: Is there a reason to keep the rest of the `DecodeEntities` function? Or could the loop just be replaced by `html.UnescapeString` as a whole?
florentsolt: @Necoro I also though about that, but sadly no. Because in XML you can declare your own custom entities, for ex `&foo;` you can't only use `html.UnescapeString`
Necoro: Yes, but `DecodeEntities` does not deal with those. Up to your patch, `&foo;` would end up with an error -- all the other code paths in it deal with "normal" entities:
- numeric constants 
- well known entitites like `&gt;` and `&lt;` and stuff.

Or am I missing something?
florentsolt: @Necoro yes you are absolutely right, that's why I said "to be tolerant" and update the test.
I do not see why, ignoring unknown entities can break something, or am I missing something?
Necoro: @florentsolt I think, we are not hearing one another ;). I was just advocating for replacing the whole else block starting at line 122 by one call to `html.UnescapeString`, because it does nothing that UnescapeString does not also do. 

And perhaps, even more code can be removed.
florentsolt: @Necoro ahah :) got it! Yes, that will do. Replacing the block starting at line 122 by `html.UnescapeString` will do the trick.

I can update this PR, but do you accept those behaviours:
```
--- FAIL: TestDecodeEntitiesInvalid (0.00s)
    parseutils_test.go:53:
        	Error Trace:	parseutils_test.go:53
        	Error:      	Expected value not to be nil.
        	Test:       	TestDecodeEntitiesInvalid
        	Messages:   	"&#;" was decoded to "&#;"
    parseutils_test.go:53:
        	Error Trace:	parseutils_test.go:53
        	Error:      	Expected value not to be nil.
        	Test:       	TestDecodeEntitiesInvalid
        	Messages:   	"&#x;" was decoded to "�"
    parseutils_test.go:53:
        	Error Trace:	parseutils_test.go:53
        	Error:      	Expected value not to be nil.
        	Test:       	TestDecodeEntitiesInvalid
        	Messages:   	"&#12a;" was decoded to "\fa;"
    parseutils_test.go:53:
        	Error Trace:	parseutils_test.go:53
        	Error:      	Expected value not to be nil.
        	Test:       	TestDecodeEntitiesInvalid
        	Messages:   	"&#xfoo;" was decoded to "\x0foo;"
```
Necoro: @florentsolt: Note: I'm not affiliated with this project, just a passer-by that looked at your patch and thought "hmm, this now seems redundant". Can't say anything about behavior changes :( -- to be honest, I figured there weren't any, thus my remark in the first place.

florentsolt: @Necoro ok, thanks for your remarks anyway, it's better now.
mmcdole: Thank you both for the contribution!

Patch:
diff --git a/internal/shared/parseutils.go b/internal/shared/parseutils.go
index 7c79b74..509934c 100644
--- a/internal/shared/parseutils.go
+++ b/internal/shared/parseutils.go
@@ -3,9 +3,8 @@ package shared
 import (
 	"bytes"
 	"errors"
-	"fmt"
+	"html"
 	"regexp"
-	"strconv"
 	"strings"
 
 	xpp "github.com/mmcdole/goxpp"
@@ -119,47 +118,7 @@ func DecodeEntities(str string) (string, error) {
 			buf.Write(data)
 			return buf.String(), nil
 		} else {
-			if data[1] == '#' {
-				// Numerical character reference
-				var str string
-				base := 10
-
-				if len(data) > 2 && data[2] == 'x' {
-					str = string(data[3:end])
-					base = 16
-				} else {
-					str = string(data[2:end])
-				}
-
-				i, err := strconv.ParseUint(str, base, 32)
-				if err != nil {
-					return "", InvalidNumericReference
-				}
-
-				buf.WriteRune(rune(i))
-			} else {
-				// Predefined entity
-				name := string(data[1:end])
-
-				var c byte
-				switch name {
-				case "lt":
-					c = '<'
-				case "gt":
-					c = '>'
-				case "quot":
-					c = '"'
-				case "apos":
-					c = '\''
-				case "amp":
-					c = '&'
-				default:
-					return "", fmt.Errorf("unknown predefined "+
-						"entity &%s;", name)
-				}
-
-				buf.WriteByte(c)
-			}
+			buf.WriteString(html.UnescapeString(string(data[0 : end+1])))
 		}
 
 		// Skip the entity


Test patch:
diff --git a/internal/shared/parseutils_test.go b/internal/shared/parseutils_test.go
index c80842d..b09b239 100644
--- a/internal/shared/parseutils_test.go
+++ b/internal/shared/parseutils_test.go
@@ -23,6 +23,7 @@ func TestDecodeEntities(t *testing.T) {
 		{"&#34;foo&#34;", "\"foo\""},
 		{"&#x61;&#x062;&#x0063;", "abc"},
 		{"r&#xe9;sum&#x00E9;", "résumé"},
+		{"r&eacute;sum&eacute;", "résumé"},
 		{"&", "&"},
 		{"&foo", "&foo"},
 		{"&lt", "&lt"},
@@ -38,24 +39,6 @@ func TestDecodeEntities(t *testing.T) {
 	}
 }
 
-func TestDecodeEntitiesInvalid(t *testing.T) {
-	tests := []string{
-		// Predefined entities
-		"&foo;", // unknown
-
-		// Numerical character references
-		"&#;",     // missing number
-		"&#x;",    // missing hexadecimal number
-		"&#12a;",  // invalid decimal number
-		"&#xfoo;", // invalid hexadecimal number
-	}
-
-	for _, test := range tests {
-		res, err := DecodeEntities(test)
-		assert.NotNil(t, err, "%q was decoded to %q", test, res)
-	}
-}
-
 func TestStripCDATA(t *testing.T) {
 	tests := []struct {
 		str string