webERP Forum
DB_escape_string - Printable Version

+- webERP Forum (http://www.weberp.org/forum)
+-- Forum: webERP Discussion (http://www.weberp.org/forum/forumdisplay.php?fid=1)
+--- Forum: Development Discussion & Specification (http://www.weberp.org/forum/forumdisplay.php?fid=10)
+--- Thread: DB_escape_string (/showthread.php?tid=2143)

Pages: 1 2 3 4 5 6 7 8 9


RE: DB_escape_string - icedlava - 03-21-2014

(03-21-2014, 04:51 PM)serakfalcon Wrote: PHP 5.2 has been unsupported since 2011. Three years ago! PHP 5.3 is also supposed to be reaching end of life sometime this year. I don't think we should enable these sorts of bad practices.

imho, i believe we should retain a maintenance branch for anyone that wants to use it with php < php 5.3 if they need to. I understand that in some cases people cannot upgrade.

For best practice, security and other aspects, I would support making php 5.3 a minimum and continuing stable development in a php 5.3 and up branch.




RE: DB_escape_string - serakfalcon - 03-21-2014

(03-21-2014, 05:48 PM)icedlava Wrote:
(03-21-2014, 04:51 PM)serakfalcon Wrote: PHP 5.2 has been unsupported since 2011. Three years ago! PHP 5.3 is also supposed to be reaching end of life sometime this year. I don't think we should enable these sorts of bad practices.

imho, i believe we should retain a maintenance branch for anyone that wants to use it with php < php 5.3 if they need to. I understand that in some cases people cannot upgrade.

For best practice, security and other aspects, I would support making php 5.3 a minimum and continuing stable development in a php 5.3 and up branch.

It can't hurt. The current code is fairly stable, so if we're going to make sweeping changes anyways, why not save the current version as stable PHP 5.2 and down and continue with the changes and the latest features? We should shoot for PHP 5.4+, with the sunset of PHP 5.3 in mind.


By the way icedlava, it'd be nice to have a single place to have all the code that isn't yet in core but adds new features core doesn't have, can you put changes you've made up on GitHub or somewhere, that makes it easier to see what you've done, and from that how it can be merged with all the other stuff people are working on.


RE: DB_escape_string - icedlava - 03-22-2014

Hi serakfalcon
Quote:can you put changes you've made up on GitHub or somewhere, that makes it easier to see what you've done, and from that how it can be merged with all the other stuff people are working on.

I usually commit anything that can be committed to webERP. On other areas such as TWIG, I have discussed it I think some time last year I recall, but at this stage there is no plans to put it into webERP.

In the case of these changes with bind variables and so on, that go across the code base, I think we need a very planned approach, script by script.

By having both DB_query and DB_bindquery we can work consistently through a list, with these functions both working concurrently until we finally can eliminate DB_query.

The other alternative is to have a totally separate branch - development branch - where all the new code is added. Given we are in subversion I'm not so keen on this. It doesn't have the convenience of GIT or MTN or other distributed systems for propagating/merging etc of branches.

So far we have the DB_bindquery and audit trail code (was the user id issue sorted?). We probably still need to look at how we end up prepping output for the browser.

Cheers,



RE: DB_escape_string - serakfalcon - 03-22-2014

I was thinking about the implementation of this and did some digging, Doctrine DBAL (http://jwage.com/post/31080076112/doctrine-dbal-php-database-abstraction-layer ) looks very 'webERP'-y,
you can do things like:
Code:
$qb = $conn->createQueryBuilder()
    ->select('u')
    ->from('users', 'u')
    ->where('u.id = :user_id')
    ->setParameter(':user_id', 1);
it supports MySQL, Oracle, Microsoft SQL Server, PostgreSQL, SAP Sybase SQL Anywhere, SQLite and Drizzle, and has a larger developer base than webERP so it's definitely good-quality code. If we're already rewriting the database code I would suggest using this instead.

(03-22-2014, 12:31 AM)icedlava Wrote: So far we have the DB_bindquery and audit trail code (was the user id issue sorted?). We probably still need to look at how we end up prepping output for the browser.

Yeah, with the code I posted there's no trouble with the user id, it's just, if we used logging at the database level, we could catch any alteration of the database, which is why I suggested it, but then we can't be sure what user did it, so I think sticking with the status quo is best. It just means we have to be careful to include audit trail code in every API we write to access the database, that's all.


RE: DB_escape_string - phil - 03-22-2014

I am not keen to have two functions for the same thing - if we are to convert the DB sending the parameters for escaping then we must do this everywhere in the code... it is all or nothing. We cannot have it half and half. We have to make a new branch and do it..... or not.


RE: DB_escape_string - serakfalcon - 03-22-2014

(03-22-2014, 03:20 PM)phil Wrote: I am not keen to have two functions for the same thing - if we are to convert the DB sending the parameters for escaping then we must do this everywhere in the code... it is all or nothing. We cannot have it half and half. We have to make a new branch and do it..... or not.

Ok then let's do it, how do you want to split things up?




RE: DB_escape_string - phil - 03-23-2014

http://www.weberp.org/forum/showthread.php?tid=2143&pid=5118#pid5118

We need the final form of the function ...

1) I would like to understand fully... and document what is required to be modified for each call to the new DB_query($SQL,$Parameters:array) - so we need the actual code for this function finalised - here. Are we all happy with Jo's http://www.weberp.org/forum/showthread.php?tid=2143&pid=5170#pid5170 - I guess I would like this to be webERP style with brackets on the same line as the function call - ProperCase variable naming etc .... but largely happy.

2) I will create a new branch

3) I can grep DB_query( and make a new thread - I expect there will be Jo/yourself and me doing the work - and there will be a LOT of it. I can allocate names to scripts if you like.

As previously noted and I remain unconvinced, I am not keen on adding frameworks or abstracting tables to a class - which to my mind provide only limited advantages at the cost of readability.




RE: DB_escape_string - serakfalcon - 03-23-2014

The code I posted before requires the DB change below, as it no longer passes a time value but lets the DB set the timestamp on insert.

the parameters:
Code:
($query,
                    $bindvars = null,
                    $ErrorMessage='',
                    $DebugMessage= null,
                    $Transaction=false,
                    $TrapErrors=true)
Code changes: &$Conn is no longer passed instead the global $db is used inside the function.
Every value to be added to the DB should be replaced by a ?, and instead added to the array $bindvars in order of appearance in the code.
so $bindvars[0] represents the first question mark, $bindvars[1] the second and so on. I recommend using an associative array of array('columnname' => 'value') which will help any future implementation of PDO but it's not required.
All the other parameters work the same as before, except there are new errors if the statement could not be prepared and variables could not be bound.

The return values will be an associative array of all values returned by the query, the first key being row and the second key being by column name (in order as per the statement)
so for example in
Code:
SELECT transactiondate, userid, querystring
FROM audittrail LIMIT 5

$returnvalue[0]['transactiondate'] will be the transactiondate of the first column, but it could also be accessed as $returnvalue[0][0].
So essentially we replace the while loops with a foreach loop.

Instead of pre-fetching all elements into an array, we could pass the statement as the result of the function, however if we do that then we need to remember to close the statement after we're done with it. in this case it would be
Code:
while($myrow = $returnvalue->fetch()) {
//do stuff
} //end while
$returnvalue->close();
I don't like the idea of relying on people to close things so that's why an array, but it is a matter of preference and is simple to alter.

Other notes:
The audit trail stores the params as JSON which strikes a balance between being able to be recalled by the code, and being human readable (the alternative would be to iterate through the array and replace '?' with each variable, but it won't work properly if question marks are included in any fields (for example if we had field notes and inserting the comment 'Follow up?', the next string replace call will replace that question mark, so either we write some sort of parser or come up with a regexp that we are certain will not come up as input, or just keep them separate.)
So that's why JSON. Naturally, any code that displays the audit trail will need to show the new params column, so that would be another change.

Code:
ALTER TABLE `audittrail`
CHANGE COLUMN `transactiondate` `transactiondate` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ,
ADD COLUMN `params` TEXT NULL AFTER `querystring`;

As a side note on Doctrine DBAL, you can write it almost exactly like a PDO statement, except that it supports many different DB types out of the box, so while I do prefer abstraction to make things easier in general, in this case Doctrine DBAL doesn't abstract much as far as the queries are concerned, but automatically converts the query to fit the particular database you want, which would expand the amount of DB's supported by webERP significantly at negligible cost. With the classloader, it only loads classes you use so it is fairly lightweight. In this case we could even use named parameters with the mysqli driver!


(03-23-2014, 09:00 AM)phil Wrote: As previously noted and I remain unconvinced, I am not keen on adding frameworks or abstracting tables to a class - which to my mind provide only limited advantages at the cost of readability.

You have very strange opinions but I will respect them in this case regardless.




RE: DB_escape_string - Uhuru - 03-23-2014

(03-23-2014, 09:00 AM)phil Wrote: I expect there will be Jo/yourself and me doing the work

As always I am happy to help as well.

Tim


RE: DB_escape_string - icedlava - 03-26-2014

Hi Phil,

I have added a preliminary DB_query function to the new branch along with a few support functions.

I have tried to keep a consistent signature in line with your wish to use the one DB_query function. The signature of the function is now a little cumbersome when using bind vars as the full signature must be passed along with the $Bindvar array as the last in the signature (defaulting to null if none passed).

I have not yet fixed the audit trail section when $Bindvars are passed into the function.

I have also committed the AddCustomerContacts.php script updated to use the new $Bindvars array passed to DB_query.

As per already posted code, the result set is returned as an associative array, and for a single item this is already adjusted (current($result)).

This means that any scripts using the $result set can do a foreach ($result as $myrow) instead of the usual while loop.

Note - i have also adjusted session.inc and removed the DB_escape_string use on session, post and get vars. This should no longer be required and if not removed we'll end up with multiplying slashes in the database.

Feel free to make adjustments as deemed necessary on this draft code.

Cheers,


Hi serakfalcon,
(03-22-2014, 12:55 AM)serakfalcon Wrote: I was thinking about the implementation of this and did some digging, Doctrine DBAL (http://jwage.com/post/31080076112/doctrine-dbal-php-database-abstraction-layer ) looks very 'webERP'-y,

I looked at doctrine DBAL some time ago(and propel) when working on another open source project and looking for an ORM. While it is good, it was not used. It just added another layer of code and syntax to learn, and was rather slow although the latest Doctrine is much faster. Do we really need this for webERP?

For webERP, we do still need to look at how to effectively prep the post, get and session vars, as required for output to the browser, or ajax or PDF etc.

I'm thinking of an api to enable the convenient and consistent prepping on vars without, hopefully adding too much overhead.

Cheers,