(03-10-2014, 04:16 PM)serakfalcon Wrote: I agree with Phil's descision here, temp fixes are the enemy of the good.
I would usually agree with you too, however the DB_escape function is causing a lot of issues in a client database so it's the lesser of two evils in this case at least for them.
Quote:.
A better approach would be to fix up DB_escape_string() and where it is being used so it only happens when it needs to.
That is the intent - however we cannot just 'fix DB_escape_string' without fixing many other areas in the code (see session.inc). It is related to security as you would appreciate and will take some time to go through the code base and put in place a suitable solution.
Quote: saves us from digging around for where mysqli_escape_string got used if we switch to PDO or someone wants to use PostgreSQL or SQLite or even MSSQL down the road.
Of course that is the reason we use APIs and usually --never-- call mysqli directly. Nevertheless, as it is now we have to go digging around to find where DB_escape_string is used, and should be used when we fix it. It's just as easy to do a grep and find the mysqli_escape_string imho, and meanwhile ensure we do not continue to introduce bad data into the database. IF we continue to use DB_escape_string then people are going to have to go through and find/fix their database data - that's an even more worrisome proposal.
Of course, there is the option as previously mentioned to setup yet another 'alternative' DB_escape_string function to deal with mysqli and mysql however i find introducing 'temp' functions might be easy, but in the long run it can prove more work to remove them.
I believe that a solution for this is very much intertwined with security related to database insert/update and user data in HTML, AJAX etc. There has been a big bandaid applied (session.inc) and then inappropriate fixes (in DB_escape_string) to cope with the issues it causes.
It would be wondeful if this can be discussed and a proper, agreed solution applied. Either way it will be a lot of work.
Eg My preferred method would be to:
1. Remove the incorrect code from session.inc (prepping the post vars, get vars and session vars)
2. Put in place a proper solution for processing data for database insert/updates eg use bindvars (if using mysqli) and appropriate api function to handle that across all database insert/updates, or go to PDO (won't discuss pros/cons here).
3. Put in place a solution for processing post/get vars for use in HTML or AJAX etc
While it is relatively straighforward to write the above code, it is time consuming to implement across the codebase but happy to chip in with it.
Whatever solution is chosen, should be one agreed, and taken in conjunction with other possible areas that might impact it or be impacted by it.
Also, current version of php is 5.5. For those that are using that version we need to ensure the code base remains compatible. I think the time may be coming to retain a maintenance version for php <= 5.2 and move to a current version for php >= php 5.3 (or something of that ilk)
Hopefully this might start further discussion. Thanks for the feedback so far.
Cheers,