Improve icon helper escaping logic

[demiankatz]

This PR straightens out icon helper escaping. In some cases, the icon value was being escaped in the helper and should not have been (e.g. images.phtml, where escaping the filename before looking up a file path could result in an invalid image link -- I've added a new test case to cover this scenario); in other cases, class names were not being thoroughly escape. Now, more escaping takes place in the templates to allow for predictable/consistent behavior.

TODO

• Run full test suite
• Add changelog note when merging

[EreMaijala]

I find it odd that the spaces get escaped. It reduces readability of the HTML and adds quite a few bytes when there are a lot of icons. We could some of that by escaping only any extra classes and leaving the hard-coded ones intact.

[demiankatz]

@EreMaijala, I agree with your concerns about readability and size of output, but I chose this route for a few reasons:

1.) There was a push a few years ago to more systematically use escapeHtmlAttr for all HTML attributes, and so I was following that lead.

2.) I feel that we're starting to trend toward using the htmlAttributes view helper in more places, since it makes for more readable code and reduces possibilities of common markup errors, and I'm pretty sure it will produce output that's identical to this (so doing it now this way makes tests more future-proof against refactoring if we expand use of the helper).

3.) The previous code only escaped the dynamic classes, which actually resulted in markup that I found less readable because of the mix of regular and escaped spaces -- i.e. I understand that icon icon--font fa fa-foo is an escaped string, but icon icon--font fa fa-foo forces me to stop and do more complicated mental parsing to understand. (Maybe that's just me, though!)

I can think of a few ways to address your suggestion, but I think they all have trade-offs:

1.) Use more targeted escaping on classes in the code -- but as I noted above, this will be inconsistent with the behavior of the htmlAttributes view helper (so fixing it here doesn't fix it more generally and feels arbitrary), and it will also make the template code a little less straightforward, which raises the question of whether we want to prioritize simple/readable templates or simple/readable output.

2.) Create a new escapeHtmlAttrExceptSpaces view helper -- but that seems silly, and would add extra processing time to everything.

3.) Customize the existing escapeHtmlAttr view helper to escape everything except spaces -- this would fix the problem globally, but at the cost of doing things in a non-standard way. I'm also not 100% sure there isn't some situation where escaped spaces improves security (though I can't think of one).

What do you think? Does this change your mind about keeping things as-is, or can you think of a better compromise?

[EreMaijala]

@demiankatz Okay, fine, I can live with this. :) Your arguments are solid. Maybe we should revisit this in a more general context at some point.

I had to do some digging, and finally found the reason for escaping spaces from one of the tests: "/* Encode spaces for quoteless attribute protection */". This would be useless for htmlAttributes helper when it makes sure that the attributes are quoted.

[demiankatz]

Thanks for doing the research -- that is a good reason to do the escaping, though as you say, it doesn't really apply to us (at least, as long as we don't forget to quote our attributes, which is one bug that I've never seen occur in VuFind... but there's a first time for everything). It's too bad that the escapeHtmlAttr helper doesn't have configurable options for things like this. But I guess we could theoretically try to contribute such a feature in the future if we thought it worth the time to implement.

[EreMaijala]

@demiankatz As far as I can see, anyone not quoting attributes, especially ones with spaces, would deserve trouble. :)