This was discussed. This function basically convert html entities and quotes(depending on settings) to something else. If we knew all translated functions are outputted to the screen, then this would be fine. However, this isn’t the case. Some translations are used within the algorithm and some may get placed in the database; the escaping would then screw these processes up. Once the entire codebase uses this function, it will no longer be hard to read. This simple function around every variable that a user can “touch” will put an end to cross-scripting attacks. It’s that simple.
Considering the work that’s already been done (which were all initially posted for review); this ship has already sailed.
Also, as an aside, easier with this spacing (which I’m now using):
Thomas,
I also modularized some of the messages.php script and blindly (unable to test) fixed a bug in the interface/main/messages/lab_results_messages.php . It’s a simple bug fix, but would be great if you could do a sanity test to ensure the lab reminders still work fine.
Revisiting this thread because of it’s mention in the OEMR 5 discussion.
With regard to the readability of xl/htmlspecialcharacters, why not add another wrapper function (xlh) that one could use when translating a string that you are certain is output as html. This would simply and improve readability. Also, if we encapsulate the function it would be much easier to use.
A developer still has to know when to use xl versus xlh (instead of the old method xl + htmlspecialchars), but it’s a 1 character change in the code rather than 20 or so.
Haven’t revisited this topic for some time. Nice to see it get some attention.
Rod, wouldn’t rec incorporate htmlspecialchars into the xl() function ( see my post #21 above for reason).
Yehster, a year ago I was opposed to creating new custom functions because during the code reviews it really seemed like developers had a much more difficult time using custom functions than the standard php functions (likely because looking them up is not accessible as the standard php function via google). Since then, though, Aron on the CDR GUI did convince me that at least abbreviating the htmlspecialchars made sense, and he actually has this code in this script here interface/super/rules/include/common.php, which worked well in the CDR admin module :
/**
* This is a shortcut for htmlspecial vars.
* @param <type> $txt
*/
function out( $txt ) {
return htmlspecialchars( $txt , ENT_QUOTES );
}
Then is out(xl()). I’d rec not trying to combine it into the xl() function or else again there will be lots of errors by developers(note that even a simple thing such as the ‘e’ parameter in the xl() function (this is now depreciated) eludes most developers; I think because most developers mimick code/patterns and don’t look up the actual functions). Also a bit complicated because occasionally need to use addslashes rather than out and also have some other custom xl functions in library/translations.inc.
-brady
Off-Topic, but the “e” parameter to xl() and some other functions was a pretty foolish but of coding. “xl(stuff,‘e’)” saves a grand total of one (1) character over “echo xl(stuff)” and is actually harder to type. It also made the xl function be not well-typed since the return type of the function depended on not just the types of the arguments but also their values.
I don’t think the result of xl() should automatically be HTML-escaped, but it would be nice to have both an abbreviation for htmlspecialchars AND an abbreviation for xl+htmlspecialchars. I’ll add this to my TODO list.
hi,
I’m neutral on the combination function, but it won’t hurt anything to have around.Since ‘out’ has already been used in the CDR Admin framework to wrap htmlspecialchars, would recommend the following:
out for the htmlspecialchars wrapper
out_xl (or something like this) for the htmlspecialchars/xl combo wrapper
My branch should be ready, I’m trying to find a up-to-date OpenEMR installation to test it on. Once I run it through some simple tests, I’ll open a patch tracker item and start my review clock.
Tested out using the sweet setup at FAIKVM.com. One fix required (gitorious link above has already update), because I haven’t written PHP code in… a while. Created a tracker item
The hope is that we finalize a standard function set that we begin to use throughout the entire codebase (including for the Security project code walk-through).
i’m in favor of letting Stephen commit his stuff, myself. I do think that the out() function name is a bit less intuitive that text() but I’m ok with either or both for now. No particular opinion about the ENT_QUOTES as I’m one of those guy that has no idea what it’s for.
-Tony
