Possible bug with new version of purell

[mna]

Hi,

I'm the author of the purell package used by hugo (https://github.com/PuerkitoBio/purell). There's a fix for a bug with relative paths in purell that apparently causes issues for hugo, so I thought I'd give you a chance to look at it before I merge it into master. The fix is in the fix-relative-paths branch.

To give an idea, this test currently fails in master:

• foo/bar with flag FlagsAllGreedy does not return foo/bar, but /foo/bar (rooted path)

The changes in the fix-relative-paths branch fix this case, but it came to my attention via @anthonyfok that this broke the hugo tests. It is possible that hugo somehow relies on the buggy behaviour of purell.

If you could run a few tests with the fix-relative-paths branch and confirm to me when all is good, we can synchronize the merge to master and reduce the chance of breaking stuff in the wild.

Thanks,
Martin

[mna]

There's another fix in this branch that I just merged, it fixes a bug with Go's stdlib parser that unnecessarily encodes some reserved URL characters (see PuerkitoBio/purell#7 and golang/go#5684). I doubt this one should cause issues to hugo, but anyway it is included in this branch.

[anthonyfok]

Thank you @PuerkitoBio for the heads up!

For reference, here is more background information: PuerkitoBio/purell#5

The go test ./... error Hugo encountered may be seen in the Travis CI test build on January 22, 2015, for example: https://travis-ci.org/spf13/hugo/jobs/47995348

When Hugo (latest HEAD) is built with purell's fix-relative-paths branch, go test ./... exhibits the following errors:

--- FAIL: TestTaxonomyNodeMenu (0.00s)
    menu_test.go:341: [0] Wrong result from IsMenuCurrent: false
    menu_test.go:341: [1] Wrong result from IsMenuCurrent: false

and

--- FAIL: TestPermalink (0.00s)
    page_permalink_test.go:73: Test 0: Expected abs url: /x/y/z/boofar/, got: x/y/z/boofar/
    page_permalink_test.go:83: Test 0: Expected rel url: /x/y/z/boofar/, got: x/y/z/boofar/
    page_permalink_test.go:73: Test 1: Expected abs url: /x/y/z/boofar/, got: x/y/z/boofar/
    page_permalink_test.go:83: Test 1: Expected rel url: /x/y/z/boofar/, got: x/y/z/boofar/
    page_permalink_test.go:73: Test 2: Expected abs url: /x/y/z/boofar/, got: x/y/z/boofar/
    page_permalink_test.go:83: Test 2: Expected rel url: /x/y/z/boofar/, got: x/y/z/boofar/
    page_permalink_test.go:73: Test 5: Expected abs url: /x/y/z/boofar.html, got: x/y/z/boofar.html
    page_permalink_test.go:83: Test 5: Expected rel url: /x/y/z/boofar.html, got: x/y/z/boofar.html
    page_permalink_test.go:73: Test 6: Expected abs url: /x/y/z/boofar.html, got: x/y/z/boofar.html
    page_permalink_test.go:83: Test 6: Expected rel url: /x/y/z/boofar.html, got: x/y/z/boofar.html
    page_permalink_test.go:73: Test 7: Expected abs url: /x/y/z/boofar.html, got: x/y/z/boofar.html
    page_permalink_test.go:83: Test 7: Expected rel url: /x/y/z/boofar.html, got: x/y/z/boofar.html

More investigation is needed. To be continued... ;-)

[mna]

Looking at the test, the new results seem OK, the input is x/y/z/boofar.md, so it should return a relative path, not an absolute one. You'll notice that the test cases 3-4 and 8+ work fine, and it looks like those are effectively absolute paths (there's a "base" value).

So I think the new test results are correct. Keep in mind that I don't know the viper package, though, this is just an impression by looking at the test itself.

[anthonyfok]

@PuerkitoBio, I would agree that the Purell's new behaviour is more correct. However, it is not just the test cases in Hugo that would fail—we do not want to break any existing Hugo sites either, as the use of {{ .Permalink }} and {{ .RelPermalink }} etc. are almost universal.

I am still pretty new to Hugo (and to Go) myself, so it is taking me a bit of time to study the relevant Hugo code as well as testing a few Hugo generated web sites and see how things may break. Besides, there is now a new test error menu_test.go:341: [0] Wrong result from IsMenuCurrent: false which I do not yet understand.

A quick git grep purell reveals that the only call to purell function is purell.NormalizeURLString() in hugo/helpers/url.go, so my initial guess is adding some code to add a leading slash / nearby would provide a quick fix. Still studying. Sorry for the delay.

I do think that it would be the best interest for both projects that this change is dealt with soon. Hugo v0.13's release is imminent (likely within a few days), and so is Purell's fix of relative paths, so it is better for Hugo to adapt to Purell's new behaviour before the v0.13 release, otherwise anyone who tries to compile Hugo v0.13 would have a buggy hugo binary. 😉

@PuerkitoBio, please allow us one or two more days for us to find a solution, and once we have a fix for Hugo ready, you may then merged fix-relative-paths in Purell, and we will then commit the fix for Hugo right away. That's the plan anyhow. :-)

Thanks again for your patience, and thanks to your wonderful purell library!

[mna]

Oh sure, no problem, AFAIK hugo is the most popular user of purell, no point in rushing in a change that would break it. The sooner the better, obviously, since it fixes bugs, but let me know when you're ok with the merge.

[anthonyfok]

@spf13: Thank you for merging #902!

@PuerkitoBio: Hugo is now ready for the fix-relative-paths branch of purell, so please go ahead and merge the relative path fixes to your master branch. Thank you for your patience!

[mna]

Thanks @anthonyfok, this is merged. Let me know if you have any issues.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.