Home » Text » Text_Diff » Bug #5472

[2005-09-21 12:29 UTC] ciprian at zuavra dot net

Description:
------------
Version 0.1.0 introduced the _encode() member function which is always used forcefully from _lines(), _added() and _deleted().

Only _lines() accepts an extra parameter which can control whether the encoding is performed, but the user has no way of controlling the way it is called. _added() and _deleted(), on the other hand, don't even have such a parameter.

Making a diff renderer mangle the output in such a subjective way is highly disturbing. The output should be in pristine condition when it returns to the user code. If the programmer decides that they want to do something else with it, it's their business. They should apply htmlspecialchars() themselves, before or after the text is fed to the renderer.

My first proposal is to revert the forced encoding, as it has no business being there in the first place.

The workaround would be to allow the programmer to control the use of _encode() through a class member or contructor parameter. However, I see no reason to complicate a problem which should not have appeared at all.

Test script:
---------------
Any inline diff attempted on text that contains characters that are affected by htmlspecialchars().

Expected result:
----------------
The text WITHOUT filtering applied.

Actual result:
--------------
htmlspecialchars() is applied to the result.

[2005-09-21 14:52 UTC] ciprian at zuavra dot net

I can't see where the assumption that the inline renderer should produce HTML comes from. The package is called Text_Diff, right? Furthermore, _encode() assumes that the output would be displayed on Web pages. Why? I first used the inline renderer for wiki text, which is a simple, non-HTML, format, as a matter of fact.

If you mean that not escaping HTML would mix with the <del> and <ins> markers, that's the programmer's problem. Let them use different markers or deal with it some other way.

I also don't see how not escaping HTML would break the renderer. The escaping is done post-processing AFAICT. I commented it out and it still worked fine. Any examples for which it wouldn't?

I still maintain that messing with the text fed to the renderer is a nasty thing to do. At the very least, consider making it an option. Consider also NOT applying it by default. Breaking users' code for arbitrary assumptions is not nice.

[2005-09-21 16:49 UTC] chagenbu at php dot net

Would it hurt for you to take a _slightly_ less confrontational tack on this?

With the default markers being HTML tags, the diff output _must_ be escaped internal to the renderer. I honestly don't remember why escaping beforehand didn't work, but I know I considered it. Escaping after means you escape the markers as well, and lose the ability to tell the markers apart from any <ins> or <del> tags that are part of the actual diff.

I'd be all for having a setting on the renderer for whether to output XHTML or plain text. You seem to be quite motivated to do this and to have spent a lot of time with the code, so if you were to submit a patch to add this option I'm sure it would be committed quickly. Otherwise it'll stay on the todo list for the package until someone has time to do it.

[2005-09-22 07:26 UTC] ciprian at zuavra dot net

Fair enough, and sorry if I seemed too agressive.

Allow me to explain my circumstances. Perhaps my latest case was a bit unusual. I was actually trying to obtain a diff which combined the markers and XHTML at the same time, unescaped. I know it's not valid XHTML, but that's what I needed: a kind of poor-man's htmldiff, if you want. You can see how _encode() would hurt in this particular case.

Give me a few days, I'll be back with a patch.