I came across an interesting piece of PHP code which has me a bit stumped as to why the author has chosen to do this.
function do_something($db, $post_vars){
foreach($post_vars as $key => $value{
$vars[$key] = mysqli_real_escape_string($db, $value);
}
return $vars;
}
$db = mysqli_connect("myhost","myuser","mypassw","mybd") or die("Error " . mysqli_error($link));
do_something($db, $_POST);
It got me thinking about why someone would want to pass $_POST as a variable and just not access it directly inside the function? The only benefit I could think of (and this was a bit of a long shot) was if we were to append other information to $_POST before calling the function (such as):
function do_something($db, $post_vars){
foreach($post_vars as $key => $value{
$vars[$key] = mysqli_real_escape_string($db, $value);
}
return $vars;
}
$db = mysqli_connect("myhost","myuser","mypassw","mybd") or die("Error " . mysqli_error($link));
foreach($_POST as $post_key => $post_value){
$post[$post_key] = $post_value;
}
$post['my_custom_var'] = "a";
do_something($db, $post);
There is, however, no evidence of this practise anywhere in the code. Just calls to do_something() with $_POST being passed as an arugment.
My question then is, is there any benefit in doing it like this that I've missed or did the author simply not understand that $_POST is a global variable?
A complete long shot: Is there perhaps even any well intended "later additions" they could make to this (such as my example) that would almost justify this practise or is this just a case of misunderstanding. Or perhaps is there a security implication that could justify the practise?
IMHO it's a practice of abstraction, and there are benefits:
Generality: by receiving $_POST as a parameter, the function becomes less tightly coupled to $_POST. The function may serve more scenarios & possibly be more reusable.
Inversion of control: because the function's dependency($_POST) is injected from outside, you have more control over the function. It is a bit unlikely but let's suppose your form has been updated and now you need to submit via GET method. Without modifying the function's body, passing in $_GET on the caller's side, is enough to reflect the change.
Test fixture isolation: To mock FORM inputs to test a certain code path in the function, it is better access global states (such as $_POST) in an abstracted way, so that the test itself does not bring side effects to other part of the system.
What he does is escaping every POST variable through mysqli_real_escape_string function, which makes sure you avoid SQL Injection attacks.Then he returns the array (with escaped values) by calling : return $vars;
He might have used a function to do this because he intends to repeat the steps several times throughout the application and he might just call that function and passing the variables instead of writing the code over and over again.
MAKE SURE YOU ALWAYS ESCAPE USER INPUT BEFORE MAKING ANY DATABASE CALLS.
In general, there is a very good reason to pass $_POST or $_GET or whatever array you want as argument instead of accessing it through the auto-global variables. It's called "dependency injection" and it's a key feature for testable code.
Another reason to not use auto-globals (or global variables, in general) is the readability of the code.
Compare:
function do_something($db, array $post_vars) {
// many lines of code here, you don't want to read them
}
do_something($db, $_POST);
with
function do_something_else($db) {
// many lines of code here that use $_POST, you don't want to read them either
}
do_something_else($db);
In the first case its clear that function do_something() operates over the values of $_POST (or $_GET or whatever other array full of data you pass it as argument). You don't have to read the function's code to know that; all the relevant information is displayed in the function header and in the example usage.
In the second case, the behaviour of function do_something_else() depends on the content of $_POST but one cannot know this without looking at its code.
Back to the code you posted, it doesn't look like it was written having testability in mind.
Take a look only at:
foreach($_POST as $post_key => $post_value){
$post[$post_key] = $post_value;
}
It basically does $post = $_POST in more words.