tmccormi wrote on Tuesday, October 20, 2009:
I have posted a patch for 3.1/3.2 that is a replacement for backup.php. Details in the patch record.
https://sourceforge.net/tracker/?func=detail&aid=2882124&group_id=60081&atid=493003
tmccormi wrote on Tuesday, October 20, 2009:
In case that was unclear, it works in Windows AS well as Linux and is configurable from globals.php
Rod or Brady please review it for me 
sunsetsystems wrote on Tuesday, October 20, 2009:
Tony, I’m very pleased that you are working on filling this gap.
Some questions/issues:
(1) Is this compatible with the restore script at contrib/util/restore?
(2) Do you have a program, script or documentation for restoring under Windows?
(3) We can’t toss out the new configuration backup/restore feature as this is important to IPPF, which as you know has been a sponsor of many vital improvements to the project. But as discussed before, I do plan to make the feature optional.
(4) It appears you’re skipping the SQL-Ledger database backup for those rare cases where SL is still used. This would be a very nasty surprise at restore time, so I think it’s better to treat that case as a fatal error than failing silently. But surely there is a pg_dump for WIndows?
tmccormi wrote on Tuesday, October 20, 2009:
Rod,
1) should be compatible with the restore script, we did not change the format of the backup at all, just how it was created.
2) Restore was not part of this project, so, nothing there yet, though many have made posting on that topic recently.
3) I’ll put it in for a 3.2 release, but it should be OK to release with out this feature for a 3.1 patch, don’t you think?
4) The SQL ledger stuff will get backed up if the system can find postgres installed etc. So that shouldn’t be an issue. The Windows installer on the site does not install postgres. We did not (intentionally) take any any features that were in the 3.1 release away. Bugs are always a possibility ….
sunsetsystems wrote on Tuesday, October 20, 2009:
(1) No, it appears it’s not the same format. The current backup produces a tar file with gzipped components. This one produces a gzipped tar file. So the restore script will surely not work. I prefer the gzipped components because that way less decompression needs to be done in the early stages of a restore.
(2) OK, but it’s not wise to depend on a backup procedure without having tested the restore procedure. So I’d strongly encourage you to at least write up manual restore instructions as part of your patch, otherwise there will be a lot of asking in the forums.
(3) True that the config backup/restore is not part of 3.1, however (1) needs to be resolved (and tested) first.
(4) Check again please. My concern is this part:
if ($form_step == 3) {
// This is skipped if Windows, because pg_dump ain’t there.
if ((!IS_WINDOWS) && $GLOBALS &&
$GLOBALS !== 2)
{
$form_status .= xl(‘Dumping SQL-Ledger database’) . "…<br />";
…
Thanks!
tmccormi wrote on Tuesday, October 20, 2009:
Thanks we’ll tweak.
-Thanks
bradymiller wrote on Wednesday, October 21, 2009:
hey,
May be just better to get your changes working with the development tip stuff(Rod’s new stuff shouldn’t interfere at all with your changes); then Rod could add his own fixes (turning off his stuff by default) on a separate commit. By doing these on separate commits then easy to put this into the 3.1.0 when ready. On that note, in order to be feasible for 3.1.0 there really need to be no required changes to globals.php, since my mechanism for patches is simply overwriting files (easy cross-platform process of opening a zip file requires very minimal resources to support). To reduce your globals footprint really don’t need the php version stuff there (see my message in tracker for another function you can use for this). Also,seems odd that the php temp directory is unavailable in earlier php version; if figure out way to get around this could then could remove even more code. Hopefully just get stuck with dealing with windows check and the mysql directory, which would be feasible for me to just move this stuff temporarily to backup.php for 3.1.0 only after you get finished up and commit to the development tip.
-brady
bradymiller wrote on Wednesday, October 21, 2009:
Rod and Tony,
On an unrelated issue, what is happening to the file in the tmp directory after it’s downloaded? Is it removed? If not, this seems really insecure. If this is the case, seems like either placing a warning that this file exist at location and/or even better a delete button to remove it after download complete.
-brady
sunsetsystems wrote on Wednesday, October 21, 2009:
Brady, the temporary file is sent via the backup script and should not be directly readable from a browser. Is that what you were concerned about?
brbill wrote on Wednesday, October 21, 2009:
Hi guys, I’m Bill. I am the author of the changes in these files, so I’ll address this from here.
(By the way, this is retry #2 at attempting to post this. Don’t left-click on that "Markdown syntax" link while editing, or you’ll navigate fully away and when you come back the note you were composing will, er, decompose.)
**Rod wrote:**
*(1) No, it appears it’s not the same format. The current backup produces a tar file with gzipped components. This one produces a gzipped tar file. So the restore script will surely not work. I prefer the gzipped components because that way less decompression needs to be done in the early stages of a restore.*
This is true. I’m currently restoring the format to the way it was. It was an intentional change, but I wasn’t fully aware of how restores were done, and I’ll accept my lumps for that. I’ll make sure that restore works in windows too. This means more changes, because I’m sure that restore uses tar, which isn’t in a standard windows install.
*(4) Check again please. My concern is this part:*
if ($form_step == 3) {
// This is skipped if Windows, because pg_dump ain’t there.
if ((!IS_WINDOWS) && $GLOBALS &&
$GLOBALS !== 2)
{
$form_status .= xl(‘Dumping SQL-Ledger database’) . "…<br />";
…
Not sure what the objection is here. To the existing and expression, I just anded a check for if this is Windows. If not windows, the code runs as it always has. If it is windows, we fall to the else statement, so that we can go on to the next step.
I’d defined IS_WINDOWS in globals.php, because there are going to be a bunch of places where it is needed going forward. I’m sure you guys realized this.
Could I get a clarification of what the problem is with #4?
**Brady wrote:**
*Also,seems odd that the php temp directory is unavailable in earlier php version; if figure out way to get around this could then could remove even more code.*
I wouldn’t say odd. There was just no such function implemented up until 5.2.1 of PHP. It’s the only function out there for cross-platform determination of the temp directory. You can’t rely on $TEMP or %TEMP% or /tmp for cross-platform use.
If what we’re saying here is that we ALWAYS want to specify the temp directory to be used specifically in globals.php for every installation, then I understand. In fact, for situations where multiple instances of openEMR are running on the same server, you probably do want to override the temp directory with a custom one (thus the code to do so).
However, I disagree with the notion that less code is always simpler. I’ll look at the version-checking function you talked about to see if I can simplify there, but for the temp directory check, cross-platform compatibility is important.
(from another thread) *3) After creating the backup, lets ensure we delete it from the tmp directory (not sure if your doing this already). Then not leaving all patient data in an unsecure area.*
To do this would mean to change how it already worked even prior to my modifications. As you noted, it might be impossible to remove the backup file while it’s uploading to the client browser. I would consider this as a separate request to be addressed later.
-Bill Cernansky / mi-squared
sunsetsystems wrote on Thursday, October 22, 2009:
The issue with (4) is the rare case where Windows and SQL-Ledger are both used. It seems you are not backing up the SQL-Ledger database in that case. As the A/R data is an important part of the system, I think it’s better to show an error message instead of leading the user to believe that the backup worked.
Even better to make it work, but again it’s an unusual situation at this point.
brbill wrote on Thursday, October 22, 2009:
**Rod wrote:**
*The issue with (4) is the rare case where Windows and SQL-Ledger are both used.*
Okay. So I should make this work. At issue: knowing how to determine if SQL-Ledger is used or not in a windows environment. I know nothing of SQL-Ledger, so if any of you have insight, I will be grateful to hear it.
**Brady wrote:**
*For the php version check in globals.php, wouldn’t it be cleaner to just
use the version_compare() function; ref here
http://php.net/manual/en/function.version-compare.php*
Yes. This is better. Change made. But if we run this function 10,000 times, it will be slightly slower. 
-Bill Cernansky / mi-squared
bradymiller wrote on Thursday, October 22, 2009:
hey,
Regarding issue (4), I’d be ok with just showing a error/warning message to windows users that are using sql-ledger (this should be a relatively rare setup). When somebody pops ups who actually needs it can then figure it out and have them test it (just testing this would be a feat for us; I and I am sure you don’t want to try to get openemr/sql-ledger working in windows).
Regarding temporary file, from my understanding of this script, a backup is created in the tmp folder and then donwloaded. This means when done the file sits in the tmp folder. I could live with this if the user/admin knew this, but most won’t know this; just seems wrong to have a lingering file liek this that admin is not even aware of on their system. Just placing a message where it is, and that is insecure would seem appropriate (a delete button option after download even better). Not related really to the multi-OS stuff so if nobody wants to take this on with the multi-OS stuff, I’ll place it in a feature request on tracker
-brady.
tmccormi wrote on Saturday, October 24, 2009:
Side note: Rod please add Bill (brbill) to the developer list, so I can add him to the Tracker for future bug/patch work. Thanks!
-Tony McCormick
bradymiller wrote on Saturday, October 24, 2009:
Tony, brbill now in developer list. -brady
ideaman911 wrote on Thursday, October 29, 2009:
Welcome Aboard Bill;
Re the Windows & SQL Ledger issue - my recall is thyat there was a problem with this from the start, as the SQL Ledger would not work within the xampp configuration and needed another install of MySQL (?). So I agree with Brady that a simple warning should suffice.
Rod; Since you rolled the A/R into the distribution, I thought SQL Ledger was no longer being supported (?) Have I not seen advice to former users, and those wanting to, to use the CSV Export functions? Just looking for a clarify, as i don’t really KNOW about it at all BECAUSE I am a Windows guy (see first half of note ;-).
Joe Holzer Idea Man 315-622-9241 im@holzerent.com
http://www.holzerent.com
sunsetsystems wrote on Thursday, October 29, 2009:
SQL-Ledger support is still in the code, and probably still works. However it seems unlikely that it will get much maintenance effort down the road.
SL can be made to work with OpenEMR under Windows (I have done it using Strawberry Perl), but it’s not easy.
tmccormi wrote on Friday, October 30, 2009:
Threads getting muddy here, but it would be way better to put our effort into standardize A/R reporting, export to spreadsheet, and other bookkeeping tools (I heard GNUCash mentioned) than keeping any SQL ledger work going.
-Tony
brbill wrote on Wednesday, November 11, 2009:
After much merging with the tip and verification of functionality and a long vacation in Boston, I’ve posted a new patch for this. Please have a look and let me have both barrels if I’ve screwed anything up.
<a href=“https://sourceforge.net/tracker/?func=detail&aid=2882124&group_id=60081&atid=493003”>https://sourceforge.net/tracker/?func=detail&aid=2882124&group_id=60081&atid=493003</a>
bradymiller wrote on Wednesday, November 11, 2009:
hey,
Got white screen death since mandriva(developer appliance) doesn’t have Archive/Tar.php . Will hopefully get that figured out soon. Things I saw looking through code:
1) This is really for Rod. Why are we using the sed stuff to replace things in the dump file. Is this really needed? Because if not, then the Windows users won’t need perl set up.
2) To simplify code and temp directories, why not just have it hard coded always and not check for the php tmp directory? I say this because losing track of these files and placing them in an unsecure area would be bad. so seems more secure and would simplify code to just always direct where it goes from globals.php.
3) The globals.php stuff will throw off end users especially the () ? : one liners. Seems like could just have a ‘windows only’ section for mysql/perl pathnames (then backup.php can then decide to use the windows path if windows setting is on and use no path if it’s linux). Then you’ll just be left with one () ? : statement for the tmp directory would could be described with several lines of comments.
4) A minor issue is use of some of your xl() calls in backup.php. Don’t place leading or trailing spaces within the actual string. This gets lost in the translation process and has potential to create redundant english constants for the translators (ie. creating the same string without the trailing space).
The biggest issue seems to be 1; would be a bummer to actually break some of the linux boxes whom don’t have Archine/Tar.php. If this package doesn’t exist I wonder if there is a way top bypass with several ‘if’ commands in linux boxes to use command line instead of Archive/Tar.php.
-brady