what started out as a simple little reply bloated out into an inpromptu brain
fart ... lots of bla .. enjoy :-)
Jason Pruim schreef:
Hi everyone,
I am attempting to add a little error checking for a very simple login
system. The info is stored in a MySQL database, and I am using mysqli to
connect to it. I have it working with the solution provided below, but I
am wondering if this is the right way to do it or if there is a better way?
at an abstract level you might consider that your function could simply
always return a boolean (true = logged in, false = not logged in) and that the
rest of the application retrieves all the other data via the session
(as opposed to returning half the data and storing half in the session)
if you choose to store everything and only return authentication state you
might also consider to abstract the storage somewhat so that other code doesn't
have to access the session data directly. we call this concept 'loose coupling'.
for instance:
function getUserInfo($key = null)
{
if (!isset($_SESSION['user']['loggedin']))
return null;
if (!$_SESSION['loggedin'])
return null;
$key = trim((string)$key);
if ($key == '')
return $_SESSION['user'];
if (isset($_SESSION['user'][$key]))
return $_SESSION['user'][$key];
return null;
}
this example still requires that the the consumer of getUserInfo() knows
the names of the relevant columns (from multiple tables?) .. this could also
be abstracted, a simple solution would be something like:
// put these in a config file, (CKEY = 'cache key' ... just a thought)
define('CKEY_USER_NAME', 'loginName');
define('CKEY_USER_LEVEL', 'adminLevel');
define('CKEY_USER_TABLE', 'tableName');
$uName = getUserInfo( CKEY_USER_NAME );
$uLevel = getUserInfo( CKEY_USER_LEVEL );
$uLevel = getUserInfo( CKEY_USER_TABLE );
... you get? ... incidentally your column names seem to be case-sensitive,
I recommend lower or upper (depending on DBMS) case only for sql entity names
for two reasons:
1. you avoid nitpicky irritations due to SQL case-sensitivity related bugs
in your code.
2. if you lowercase all entity names you can write stuff like so:
$sql = "SELECT foo, bar FROM qux WHERE abc = 1 AND def=2";
which is a little more readable than this:
$sql = "SELECT FOO, BAR FROM QUX WHERE ABC = 1 AND DEF=2";
of course it should be more like:
$sql = "SELECT `foo`, `bar` FROM `qux` WHERE `abc`=1 AND `def`=2";
using case to differentiate between SQL and entity names becomes more useful
as the queries become more complex. I also tend to then break then up into lines:
$sql = "SELECT
q.`foo', q.`bar`,
na.`foo` AS nafoo, na.`bar` AS nabar,
noo.`foo` AS noofoo, noo.`bar` AS noobar,
FROM
`qux` AS q
LEFT JOIN
`na` AS na ON na.`qux_id` = q.`id`
LEFT JOIN
`noo` AS noo ON noo.`qux_id` = q.`id`
WHERE
(`abc`=? AND `def`=?)
AND
q.`id` IN (SELECT `qux_id` FROM `quxnobbins` WHERE `nobbin_id`=?)
AND (
(`start_date` BETWEEN ? AND ?) OR
(`start_date` BETWEEN ? AND ?)
)";
My thinking with this is if more then 1 record is returned from the
database, then there is a issue... If only is returned then the
username/password matched and I can safely show them the info...
$rowcnt = mysqli_num_rows($loginResult);
we'll assume the original sql was suitably prepared (i.e. user values escaped, etc).
but why not 'fix' the query and/or table so that it will only ever return one row?
if($rowcnt !="1"){
avoid auto-casting!
if ($rowcnt !== 1) { /*...*/ }
echo "Auth failed";
die("Auth failed... Sorry");
}else{
while($row1 = mysqli_fetch_array($loginResult)) {
this 'while' is completely pointless, you know there is just one row,
no point in looping for a single iteration.
just do:
$row = mysqli_fetch_array($loginResult);
$_SESSION['user'] = $row['loginName'];
// ... etc
$_SESSION['user'] = $row1['loginName'];
$_SESSION['loggedin'] = "YES";
"YES" is not a boolean value, I think $_SESSION['loggedin'] should be
boolean (you got deja vu here also?).
check the following code to see why:
$_SESSION['loggedin'] = "FALSE";
if ($_SESSION['loggedin'])
echo "your logged in!";
$table = $row1['tableName'];
$adminLevel = $row1['adminLevel'];
$authenticated = "TRUE";
again the boolean should be boolean!
echo "<BR>authentication complete";
with regard to seperation of responsibilities: the function should
really be either attempting an authentication *or* outputting some message
regarding the result of the authentication attempt but *not* both.
in practice this means my recommendation would be to remove the echo statements
from the function and have the code that calls this function be responsible for
outputting feedback ... imagine if you need to, someday, perform an authentication
without [direct] output? or you need to change the outputted message under certain
conditions (conditions which are outside the scope of this function)?
a function should, as much as is possible, do one thing only (and do it well), otherwise,
I guess, it would be called a functions. ;-)
}
return Array($table, $authenticated, $adminLevel);
pretty much the rest of the world writes 'Array()' as 'array()' .. the convention
being that built in functions and lang constructs are always typed lowercase. some
people write things like isSet($foo); ... but they are 'wrong' :-)
I generally try to distinguish between userland and php functions by using lowercase
for php funcs and CamelCase naming schemes for userland functions.
--
"ok, porky pig say your line."
--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
