ytiddo wrote on Friday, June 11, 2010:
Andy,
I just took a look at “refactor facilities drop-down into a function”.
I only have three minor nits to pick, and they are that:
You have an extra empty line before your closing ?>, you use both C style and C++ style comments, and your description above the function is basically repeated inside the function.
Hope it helps,
Justin Doiel
acmoore wrote on Friday, June 11, 2010:
Thanks for the reviews, guys. I think this may set a record for most amount of code review comments for least amount of code change! Differences here:
* The function now actually uses the $name parameter to set the name of the HTML select element
* If you don’t specify which item should be “selected”, then the first one is.
* We use ‘selected=“selected”’ instead of just ‘selected’ to indicate which option is selected
* $facid is wrapped in htmlspecialchars()
* the options are terminated with "</option>
* I use “true” instead of “True”
* I eliminated a superfluous blank line
* I cleaned up the comments a bit
I hope this addresses all of the concerns. please let me know if there are more
The new patch replaces the old one at the same tracker: https://sourceforge.net/tracker/?func=detail&aid=3010456&group_id=60081&atid=1245239
Thanks,
-Andy
stephen-smith wrote on Friday, June 11, 2010:
You still have some “True” and “False” in the modifications that replace code with a call to your function. If you are willing to change it in the default argument value, you might as well be consistent throughout the patch.
I don’t see where $form_facility is set in interface/reports/appointment_report.php. It looks like the old code was depending on register_globals or our fake register_globals. This should be fixed. I believe the $facility variable holds what you want. It looks like most of the other places don’t depend on register_globals or our faking of it, but please double-check me.
In your new function, $facility_id is retrieved from the database and compared against $selected. Most (all?) of your calls pass in selected as something retrived from the $_POST data. When magic_quotes_gpc or magic_quotes_sybase is on, the $_POST data might have been mangled in way that the database data is not. You’ll want to pass $_POST data through remove_escape_custom in library/formdata.inc.php before comparing it to constants or data retrieved from the database. This shouldn’t be done in your function (the function shouldn’t care where $selected comes from), but rather at each call site.
In general, we shouldn’t access $_POST directly, since what is contains depends on (at least) magic_quotes_gpc and magic_quotes_sybase which varies from PHP installation to the next. The same is true of $_GET and $_COOKIE as well pre-defined variables based on any of those. I know the files you touch do this all over the place; you only need to fix the call into your function to behave properly for now.
You can’t use formData or formDataCore for this because they also prepare(/mangle) the value for insertion into the database, and you aren’t doing that with $selected.
Finally, if $selected doesn’t match one of the values in the database, and is not ‘’ or ‘0’, your code will not generated a pre-selected item. I’m not sure the best way to handle this in general, but it seems appropriate to generate a option with value=$selected, label=(xl(‘Do not change’)), and text content xl(‘Missing or Invalid’). Check out lines 73-82 of interface/forms/misc_billing_options/view.php on my prior-auth-entry branch; it’s a different foreign key, but the same general idea.
acmoore wrote on Friday, June 11, 2010:
I’m retracting the patch. I was just trying to eliminate some duplicated code. I don’t care about it this much.
nevermind.
Thanks for your help, but this is getting out of control.
stephen-smith wrote on Friday, June 11, 2010:
I assume you don’t mind if I take the patch the last yard, since you got it 99 yards down the field? Reducing the code duplication should be done, and I’m glad you were working on it.
acmoore wrote on Friday, June 11, 2010:
Of course not. Go for it.
-A
stephen-smith wrote on Tuesday, June 15, 2010:
I posted a new patch on the tracker item.
I’ve also pushed it into my review-3010456 break on Gitorious; it should be mirrored to GitHub soon.
I’m starting testing on this patch now, but I’d love a patch review as soon as anyone can get to it.
bradymiller wrote on Tuesday, June 15, 2010:
Stephen,
I’ll check it out. As an aside, can you post your script that mirrors your repos from gitorious to github?
-brady
stephen-smith wrote on Tuesday, June 15, 2010:
The mirroring script. LGPLv3 license, if you are interested.
#!/bin/sh
remote_mirror() {
git="$1"
src="$2"
dst="$3"
( cd "$git" &&
# Update source
git remote update "$src" &&
git remote prune "$src" &&
git fetch "$src" &&
# Push all found refs and tags.
git push --tags "$dst" \
$(find refs/remotes/"$src" -type f -printf "+$src/%f:refs/heads/%f\\n") &&
# Delete all refs that weren't found.
git push "$dst" $(
find refs/remotes/"$dst" -type f -exec /bin/sh -c '
src=$2
dst=$3
[ ! -f refs/remotes/"$2"/"${1##refs/remotes/$3/}" ]
' ignored {} "$src" "$dst" \; -printf ':%f\n'
)
)
}
errno=0
out=$(mktemp -t "$(basename "$0")".XXXXXXXX) && {
err=$(mktemp -t "$(basename "$0")".XXXXXXXX) && {
( exec > "$out" 2> "$err" &&
remote_mirror ~/openemr.git Gitorious GitHub
)
errno=$?
if [ $errno -ne 0 ]; then
cat "$out"
cat "$err" 1>&2
fi
rm -f "$err"
}
rm -f "$out"
}
if [ $errno -eq 0 ]; then
errno=$?
fi
rm -f "$out" "$err"
return $errno >/dev/null 2>&1 || exit $errno
remote_mirror does the bulk of the work. If I need to mirror the other way, I could just change the order of the arguments. It takes a git repository, and two remote names that are set up through Git’s remote command.
Run from cron like so: (Not copyrightable, IMO; but GNU All-Permissive license if you need one.)
# m h dom mon dow command
0 * * * * . /home/bss/.keychain/rei-sh && /home/bss/bin/git-mirrors
~bss/.keychain/rei-sh is generated by the “keychain” program available on (at least) Debian, Ubuntu, and Gentoo. Basically, it allows my shell script to access the ssh keys I unlocked during my last login. I think it can be dropped if you are using ssh keys without a passphrase.
bradymiller wrote on Tuesday, June 15, 2010:
Stephen,
Have you tested this?
I’m getting white screen death on appointment report (no php error) and soem reports are giving ‘-’ as first choice.
The code looks very nice. I’ve been contemplating placing htmlspecialchars() within the xl() function call. Hopefully others will chime in whether this is a good idea or not. Since these string almost always get outputted to the screen, seems like a good idea. Sometimes this text may be directed to pdf output I suppose, but these html characters should never be used in this situation anyways. The reason I bring this up is that your embedding htmlspecialchars() around a xl() function, and I’m wondering what happens if you call this function twice on the same string?
-brady
bradymiller wrote on Tuesday, June 15, 2010:
regarding the git repos mirroring script… awesome. thanks -brady
bradymiller wrote on Tuesday, June 15, 2010:
hey,
A bit more details on the appointment.php bug:
get this in php error log:
PHP Warning: require_once(/appointments_report.php) [<a href='function.require-once'>function.require-once</a>]: failed to open stream: No such file or directory in /var/www/html/openemr/interface/reports/appointments_report.php on line 16, referer: http://192.168.1.146/openemr/interface/main/left_nav.php
PHP Fatal error: require_once() [<a href='function.require'>function.require</a>]: Failed opening required '/appointments_report.php' (include_path='.:/usr/lib/php/:/usr/share/pear/') in /var/www/html/openemr/interface/reports/appointments_report.php on line 16, referer: http://192.168.1.146/openemr/interface/main/left_nav.php
Perhaps related to your require_once syntax. I’d suggest keeping this syntax the same as currently used in openemr to make potential codebase-wide changes easier in the future.
Looks like the ‘-’ entries were there before you patch, so not bugs.
-brady
stephen-smith wrote on Tuesday, June 15, 2010:
Nope, I’m starting my test environment with it enabled approximately now. Perhaps I fat-fingered something.
Personally, I already dislike xl() doing anything to the translated string. Depending on the font and context a back-tick (`) is very different from a single-quote (’) or double-quote ("). There is no catch-all escaping routing that is appropriate to use in xl(). Javascript in HTML uses different escaping than the rest of HTML. MySQL uses different escaping than either, though it is unlikely xl() output will be immediately inserted into the database. Our PDF library does (or seems to do) all the escaping we need for that, but output to other formats will probably require different escaping.
Undoing htmlspecialchars() is relatively easy, but I don’t know if there’s an existing PHP function for it. I think xl() should only be doing a transform if it is universally acceptable, not mostly acceptable. I think you looked at my i18n branch before. I mostly wrote the xl_only_always() function to avoid the existing mangling that xl() does to the translated string.
Applying htmlspecialchars() twice is similar to applying addslashes() twice:
htmlspecialchars("&") = “&” which HTML renders as &.
htmlspecialchars("&") = “&amp;” which HTML renders as &.
bradymiller wrote on Tuesday, June 15, 2010:
hey,
Good to know that calling htmlspecialchars more than once is a bad idea. Also agree that doing htmlspecialchars() on all xl() stuff is likely not a good idea. If your code tests well, looks committable.
-brady
bradymiller wrote on Tuesday, June 15, 2010:
btw,
you have a typo and likely wrong page call in appointments_report.php:
require_once "$srcdit/appointments_report.php";stephen-smith wrote on Tuesday, June 15, 2010:
Yeah, evidently my mind had gone while I was editing that file. The require is supposed to be for $srcdir/formdata.inc.php.
I’ve already applied that fix to my test environment. I’ll update the patch after I am done testing.
stephen-smith wrote on Tuesday, June 15, 2010:
Okay, I am done with my testing and everything looks good on a fresh install, once I fixed that momentary lapse of sanity on the appointments report page.
I’ve already updated my Gitorious branch. I’ll post a new patch to the tracker in a few minutes.
If I don’t receive any further comments, I’ll check this into CVS tomorrow. I’m taking Brady’s comment #9 as a sign-off, but I want to give others that might not be online right now a chance to raise any issues he and I missed.