yehster wrote on Monday, December 12, 2011:
We seem to be using a version from 2004. (version 4.20)
It looks like they are up to version V5.11 5 May 2010
http://adodb.sourceforge.net/
Perhaps we should look into updating. Admittedly this is a scary prospect, but in 6 years they might have addressed the encounters, sequence id bug we are seeing with XAMP > 1.7.4 Wishful thinking I know.
-Kevin Yeh
kevin.y@integralemr.com
yehster wrote on Monday, December 12, 2011:
And of interest to developers and/or projects hoping to use transactional semantics in the future, a relevant piece of the change log.
5.07 26 Dec 2008
BeginTrans/CommitTrans/RollbackTrans return true/false correctly on success/failure now for mssql, odbc, oci8, mysqlt, mysqli, postgres, pdo.
sunsetsystems wrote on Monday, December 12, 2011:
Perhaps we could figure out how to stop distributing adodb with OpenEMR, instead making it a normal dependency?
yehster wrote on Monday, December 12, 2011:
Rod,
I just ran apt-get install php5-adodb
on my system and it installed a shared library that according to the readme is the package from adodb.sourceforge.net. However, it will take a lot of exploring to figure out how to get openemr to use that shared library instead of the included files.
I have no idea how to install the shared library under XAMP.
Given that we haven’t updated that file in 7 years, I don’t think it’s worth the hassle of trying to figure out making it a dependency. The latest version was released in May of last year, and the release before that was more than a year prior. It’s a stable package and distributing it seems much easier for many reasons, including simplifying the install process.
What would we really gain by making it a normal dependency.
-Kevin Yeh
kevin.y@integralemr.com
sunsetsystems wrote on Monday, December 12, 2011:
I think it’s worth a bit of exploring before blowing off the idea. OpenEMR has many other dependencies and it’s not best practice for a project to internally include a snapshot of some other project’s code when there is a perfectly good distribution system in place for that other project. And we don’t want to have this same discussion in another year when there is yet another adodb release.
As for Windows, I think adodb already comes with xampp.
yehster wrote on Monday, December 12, 2011:
Rod you said:
OpenEMR has many other dependencies and it’s not best practice for a project to internally include a snapshot of some other project’s code when there is a perfectly good distribution system in place for that other project.
We are doing exactly this with other pieces of code. jQuery for example, which is why we ended up with at least 4 different versions included in the package, and jQuery gets updated far more frequently than adodb.
When a new version of adodb comes out, the discussion could also possibly be, “hey all this stuff stopped working mysteriously after I upgraded all my packages. I don’t know which one caused it to break.” Low likelyhood, given what seems like a high quality development effort/release cycle by the adodb folks. However, still non-zero.
Neither option for upgrading is clearly the better practice. However, the fact that I loaded the shared library on my system and reported back indicates that I’m interested in exploring that avenue.
Any technical details others may be privy to in accomplishing this task would be useful.
yehster wrote on Monday, December 12, 2011:
An additional “to do” if we try to switch to the shared library version of adodb is how to patch for existing users. Seems like moving existing users to a shared library version is more complicated than just replacing the included version.
sunsetsystems wrote on Monday, December 12, 2011:
Yep the same problem is there for some other code (Smarty is another example), but most dependencies are external. It may help to consider the question “What makes adodb special?”
jQuery is a separate discussion, and getting to be quite a mess now with all the different versions of it embedded into the project.
Not sure I understand about existing users. Is the problem that they may have to install a new dependency?
Anyway I don’t mean to sound dismissive or anything. I really appreciate that you are looking into this issue.
yehster wrote on Tuesday, December 13, 2011:
Apt installed an adodb library from 2007 on my system that predates the transaction fixes of interest. So the fact that a standard distribution method for adodb is not kept up to date further strengthens the case that including the files in the openemr package has more advantages than disadvantages.
I agree with your central premise that using external dependencies instead of bundled libraries would in general be more elegant, but I am still unconvinced that it is the correct approach in this instance.
The precedent exists for bundling libraries, it simplifies the install for end users and it gives us as developers a known stable version to work with.
yehster wrote on Tuesday, December 13, 2011:
https://github.com/yehster/openemr/tree/adodb514
Branch using newest adodb (version 5.14).
sunsetsystems wrote on Tuesday, December 13, 2011:
Uh-oh. I just looked at git history for the adodb subdirectory and there are some OpenEMR customizations in there, most notably by ViSolve for their audit enhancements. Those would have to be studied, adapted and tested. I saw them by running “gitk library/adodb” but there may be a better way.
bradymiller wrote on Tuesday, December 13, 2011:
Hi,
There has been hackery within adodb for the audit stuff and also to support UTF8 encoding in OpenEMR. Could also do a diff of the original version of the installed package with the currently installed code to find the customizations.
-brady
yehster wrote on Tuesday, December 13, 2011:
I would like to make the case that the hackery for audit logging should be moved out of adodb.inc.php and into sql.inc.
Right now, the logging is done in adodb.inc’s Execute function for all queries, which ends up recursively calling itself through auditSQLEvent(); (a function back in sql.inc)
Rather than modify adodb.inc directly we can write a wrapper for execute that accomplishes the same thing.
Alternately, we could add explict log statements to each of query functions in sql.inc
The encounter/id bug happens in newer xamp versions because the return value for getSqlLastID() we care about is getting clobbered by the id generated by logging. There is a lot of convoluted logic that tries to prevent that from happening.
I believe that by moving it out of adodb, the logic will be simpler and thus easier to understand. Our chances of figuring out what is different on xamp will be better with simpler logic.
I also feel that we should do our best to make an unmodified version of adodb work for us. Brady, any idea where the changes required for utf-8 reside?
sunsetsystems wrote on Tuesday, December 13, 2011:
Yeah we definitely want to try for an unmodified adodb. I don’t have time to study this further right now but what you’re saying makes sense to me, and unintended recursion sounds really bad.
yehster wrote on Tuesday, December 13, 2011:
https://github.com/yehster/openemr/commit/d9bbfe40b0d70811720edeac0175bd867ecc498f
Audit log is working correctly and doesn’t trounce “insertID”.
There is a lot of “clean up of extraneous code” that can come with this.
If someone has xamp >= 1.7.4 and wants to test and see if it works with audit logging enabled, that would be useful.
Still need to know what the “utf-8” changes were since I’ve likely broken them with this.
bradymiller wrote on Tuesday, December 13, 2011:
Hi,
Been awhile since did the UTF8 stuff, and am beginning to think may of not modified adodb. Best way to sort out is to diff the adodb library directory from current codebase with the codebase from a previous OpenEMR version (2.8.3 should be good). Also, as I recall, there are scripts in OpenEMR that include sql.inc but bypass the functions there and use adodb directly (this is why I think Visolve made changes for audit engine directly in adodb).
-brady
bradymiller wrote on Tuesday, December 13, 2011:
Also,
Thanks for spending time on this. From a security standpoint, also would be good to get the adodb library current.
-brady
yehster wrote on Tuesday, December 13, 2011:
I think I can address the scripts that access adodb that don’t use the sqlXXXX functions in sql.inc by subclassing the ADODB object, and setting $GLOBALS to an instance of that subclass.
As a seperate issue, it looks like the calendar has it’s own copy of ADODB. (version V3.60 16 June 2003 ) in main/interface/calendar/pnadodb
yehster wrote on Tuesday, December 13, 2011:
BTW, Rod, thanks for the gitk tip. cool feature.
yehster wrote on Tuesday, December 13, 2011:
https://github.com/yehster/openemr/commit/f2579cbf136f79b46b67fe0a18b5ef1355237c2e
Ok, I think this is a good way to handle the logging issue. I subclassed the ADODB_mysql (the default driver) and overrode the execute method. The sqlXXX functions in sql.inc go through the subclass.
I don’t see anything in the git history that suggests there were changes for utf-8. I’ll do a diff later to be sure.
There are also a lot of comments to clean up regarding the “old way”, but I think this is working now.
-Kevin Yeh
kevin.y@integralemr.com