2

I want to write a function to -log users in / check log in details- that uses $_POST['first_name '] and $_POST['password'] as parameters. I must be doing something silly. Can someone help me out??

function log_user_in($_POST['first_name'],$_POST['password']) {

if ( !empty($_POST['first_name'])  &&  !empty($_POST['password']) )  {

$first_name = $_POST['first_name'];
$first_name = mysqli_real_escape_string($dbc, $first_name);

$password = $_POST['password'];
$password = mysqli_real_escape_string($dbc, $password);

  ---mysqli queries----

session_start();
redirect_user('page.php');

} // close if all set

} // close function log user in

I get the feeling, that $_POST['information'] cannot be used as parameters/arguments in functions, I may be wrong, in which case how do I substitute them for variables or something e.g.

function log_user_in ($a, $b) {
if ( !empty($a) &&  !empty($b) )   {
$first_name = $a;
$first_name = mysqli_real_escape_string($dbc, $first_name);
$password = $b;
$password = mysqli_real_escape_string($dbc, $password);

--- mysqli stuff ---

session_start();
redirect_user('page.php');

} // close if all set
} // close function log_user_in

and then call--

log_user_in($_POST['first_name'],$_POST['password']);

Can you tell me where I'm going wrong and what I can do to improve or give me a better method, thanks people!

J

Jaiye
  • 194
  • 1
  • 1
  • 10
  • 2
    The function declaration should have a plain parameter variable name, not a `$_POST[...]` reference. Only the function call can use that. – mario Mar 20 '16 at 01:14
  • The example you give should work, ie.`function log_user_in ($a, $b) {/*Login stuff here*/}` – segFault Mar 20 '16 at 01:15
  • Your second code should work expect that you need to do the empty checks before calling the function. You should use php's password functions to check passwords and not an password = password in sql – Roland Starke Mar 20 '16 at 01:17
  • ok great, thanks guys. I will look at php's password functions. I think that the function is failing when it gets to the msqli queries - can the function definitely run mysli queries? – Jaiye Mar 20 '16 at 01:32
  • it works better now that i have removed the empty checks, thanks but its not running the msqli queries-- $q = "SELECT `user_id` from `users` WHERE `first_name` = '$first_name' and `password` = SHA1('$password')"; – Jaiye Mar 20 '16 at 01:42

2 Answers2

2

As already stated by commenters, your second code is the right one.

But it can be simplified:

  • You don't need to assign an argument to a new variable, e.g. $first_name = $a;: directly use $first_name as argument name.
  • session_start() is not needed here (you don't use any $_SESSION[...]).
  • Anyway, even if you needed it here, it should be located elsewhere (most generally at the very begin of your file), because it must exist only once and before any other places where $_SESSION[...] is used.

Last point, your redirect_user('page.php'); seems a bit weird here. In fact you should probably end your function with a TRUE|FALSE result returned (depending on your mysql stuff), then using it when calling the function.

Actually something like this:

session_start();

// ... other stuff ...

function log_user_in ($first_name, $password) {

  $first_name = mysqli_real_escape_string($dbc, $first_name);
  $password = mysqli_real_escape_string($dbc, $password);

  --- mysqli stuff ---

  return // TRUE|FALSE depending on your mysql stuff result
}

if (log_user_in($_POST['first_name'],$_POST['password'])) {
  redirect_user('page.php');
} else {
  // echo some deny message...
}
cFreed
  • 4,404
  • 1
  • 23
  • 33
  • @RolandStarke Uh... you're right. For times I use only my DB-oriented classes so I forgot some basic information! I'll correct my answer. – cFreed Mar 20 '16 at 02:51
  • @Jaiye I'd partially wrote a nonsense about `mysqli_real_escape_string()`: don't rely to this part. Anyway I edited my answer, thanks to Roland Starke who remembered it to me. – cFreed Mar 20 '16 at 02:58
2

So let's talk good coding for a moment here. Function arguments cannot be superglobals because you're defining what variables you're using, not what values they actually are. Thankfully PHP won't let you do this

function log_user_in($_POST['first_name'],$_POST['password']) {
    echo $_POST['first_name'];
}

log_user('Bob', 'Smith');

Results in

Fatal error: Cannot re-assign auto-global variable _POST in /in/INsWe on line 3

At least prior to PHP7 (which throws a different error but still won't work)

So your second function is correct. Define your arguments (which should be clear names, not just $a, so you know which data is which) and then call the function with your superglobal values. In your case, you'll also have to pass your mysqli connection as well. So let's get this right

function log_user_in ($dbc, $first_name, $last_name) {
    $first = mysqli_real_escape_string($dbc, $first_name);
    //other stuff here

    $sql = 'SELECT *
         FROM users
         WHERE first_name = "' . $first . '"
             AND last_name = "' . $last . '"';
    $dbc->query($sql);
}

So we've avoided using any globals or superglobals inside the function (called dependency injection). This lets you and anyone else who reads your code know what the data is expected to do (no chasing down another include file or guessing where a variable is set). And we've used clear variable names so we know what the data is.

Community
  • 1
  • 1
Machavity
  • 30,841
  • 27
  • 92
  • 100