benanamen

Aaarg! Forget everything I said about the index errors. I shouldn't have even been seeing it. That whole part of the thread is mute and explains why I was thinking we were not quite on the same page. We were kinda having two different conversations. I was mistakenly doing if field empty set var=. Should have been if field !empty set var=. Doing it !empty as it should have been in this case has the same effect as the parameter not existing at all, coded error gets set either way and script halts. My normal code pattern is if field false, set error, stop script after all checks run. Quickoldcar was doing if field true set var to post param. I never set vars to a straight POST params unless it has changed for some reason. That's what threw me off. IMO that formula is no good. For one, you are setting pointless variables to POST parameters that haven't changed, and second, if there is just one parameter error, a whole bunch of already useless variables have been set for no reason. I believe the proper flow would be: Trim the POST array in one step Check for empty required fields (has the exact effect if field doesn't exist at all) if error add to error array, halt with user friendly errors from array, else continue

Rather basic. Add ORDER BY column ASC or DESC. You have too many queries. Just use a join. You are also opening and closing php unnecessarily. More importantly, you hang pictures on a wall. Next time post your code.

Ok, I did some testing on 300,000 employee records. Based on my test results, the fastest option is set an index and no limit 1 No Index Average .97 .0554 1.142 1.155 1.094 .700 1.153 1.143 No Index Limit 1 Average.38 .423 .354 .388 .348 .412 .405 .333 Indexed Average.026 .046 .026 .022 .020 .019 Indexed Limit 1 . Average 031 .020 .029 .042 .038 .042 .021 .029

So Jaques1, are you saying that by indexing username there is no reason to use the Limit 1? I suppose its time I whip out some code and see what actually happens.

15 minutes is plenty. My time is either not 15 or I get into an accelerated time warp where 15 minutes seems like 5.

I used quickoldcar's code example instead of what I actually proposed in #9. Nevertheless, the resulting error is exactly the same. If you look at my code in #9 it does use empty as you say. What I proposed in #9: // Check for empty instead if (empty($_POST['field'])) { echo 'Field Empty'; } Using the isset makes it impossible to spot the issue that the expected field is not even in the form at all since it silently hides it. The only way you would ever know is by reading the code and seeing the form field doesn't exist. There will be no error in the logs, no error on the screen with error reporting on. No nothing except all the missing data you thought you were getting. I would call anything that generates a system error a bug even if it is just a notice. It means something is wrong and needs to be fixed. I would say the following is pretty meaningful as to what and where the problem is: Notice: Undefined index: lname in E:\xampp\htdocs\help.php on line 32. Whether you see it in the logs or on the screen it is quite clear. You are also not going to want to abort the script on an undefined index. The worst that is happening using your example of removing the field is your not getting that piece of data. Using isset, you will never know until you look for the data and it is not there. With what I proposed, you are at the least getting the error logged which should be being checked regularly anyways. As you said, there is going to be bugs that will make it to production. Usually found because users do crazy stuff you would never think they would do. I am pretty sure that is not what I am saying. My comments are specifically relating to whether a site supplied form field will always be isset when POST is submitted or not and if not, under what code provable conditions will it not be. I also tested in Curl and it makes no difference. Besides a form or curl, what other way is someone going to POST your expected form data? I am not saying there is not a case where what I proposed will be a problem, but I am saying I dont know of it and would like to know what it is. Same type of thing when we had our conversation about if ($_SERVER['REQUEST_METHOD'] == 'POST') VS if ($_POST). You were able to show me specific cases to support $_SERVER['REQUEST_METHOD'] == 'POST being the correct way and how it could fail otherwise. I want to know what I know, not just because someone says it and if I repeat it to someone else, to be able to back it up in code. Using your example of removing the form field and, as actual code testing shows, how are you going to know that the error check for missing lname is looking for a form field that doesn't even exist if the problem is completely hidden with the isset?

Curl test case same EXACT result. (Of course need to change the if submit line to if ($_SERVER['REQUEST_METHOD'] == 'POST')) <?php $curl = curl_init(); curl_setopt_array($curl, array( CURLOPT_RETURNTRANSFER => 1, CURLOPT_URL => 'http://localhost/help.php', CURLOPT_USERAGENT => 'cURL Request', CURLOPT_POST => 1, CURLOPT_POSTFIELDS => array( 'fname' => 'Joe', 'email' => 'user@example.com' ) )); echo $resp = curl_exec($curl); curl_close($curl); ?>

In this particular case, I deleted the lname field and changed the error check to if ($_POST['lname'] != '') { $lastname = $_POST['lname']; } else { $errors[] = "<p>Sorry, you must give your Last Name!</p>"; } What I get (with error checking on) is undefined index (ONLY) which is exactly to be expected. Firebug not even needed although I did look there. While in development mode the problem isn't going to get past you without you knowing it. In this case, if you leave the isset, you are never going to know that you are either missing the field you expect, or you are doing an error check you dont want. It will just silently be ignored. So in this instance I see it as bad to have the isset. In development, you are going to know about this and be able to fix it before going to production. If your dev environment matches the production environment, which ultimately, it should, its not going to unknowingly make it to production. I suppose in a group development with different people doing things it would serve as a safety net. It definitely isn't going to hurt anything to do it that way. Everything I have ever done I was the only programmer so I have never experienced problems from multiple coders. Only once I was coder and another guy did the design .

My bad, I meant WHERE username and the use case being a login and only one possible username match. user_id would have been the indexed auto increment which is not the column the WHERE would be checking.

Using the provided form, under what possible conditions would the form fields not be set on POST (other than unchecked checkboxes)? Is there a test case that would prove that out? I get that it is not that much more code, but I am always wanting to know what will fail and under what conditions.

I personally find the allotted time to be able to edit your post too short. Numerous times I have re-read my post and noticed some typo or something that needed changed and can no longer edit my post. Just how long is the current allowed time and can someone increase it another minute or two?

* Actually, OP has a problem in his code I used as an example. He says if firstname is not empty, set $firstname, Then he says echo '<p>Sorry, you must give your First Name!</p>';} Which is not right since first name was was not empty.

I think you misunderstood what I said because of the way I worded it. I should have said "As far as the form being submitted, all the fields are going to be isset already when the form is submitted." I didn't mean remove the error checking, just the isset, still check for empty. On submit, all the form fields other then check boxes will be isset so there is no point checking for it. It was also in specific response to the code from QuickOldCar. OP had it right. OP Example: Here is what I meant in code: <?php if ($_SERVER['REQUEST_METHOD'] == 'POST') { // Not this. It will always be isset when using provided form if (isset($_POST['field'])) { echo 'Field Set<br>'; } // Check for empty instead if (empty($_POST['field'])) { echo 'Field Empty'; } } ?> <form action="<?= $_SERVER['SCRIPT_NAME'] ?>" method="post"> <input name="field"> <input name="submit" type="submit" value="Submit"> </form>

You dont want to do this: if (isset($_POST['submit'])) { It will completely fail under certain conditions. The correct and error proof way is if ($_SERVER['REQUEST_METHOD'] == 'POST') You also dont need to do all those trims. You can trim the whole post array all at once then get rid of all those issets and check for !empty. As far as the form being submitted, all the fields are going to be isset already. You can cut down the code by at least 50%.

Facebook now uses HACK for coding. Not sure of the DB.

A few times I have seen in the case of SELECT username WHERE user_id=? there is LIMIT 1. My immediate thought was, why add the LIMIT 1 since there should only be one result anyways until I saw an explanation somewhere else which said it will stop MySQL from continuing searching the DB for more matches. A light bulb went off because this made sense to me. My question is, is that really the case? And if so, are we talking enough of a performance gain for it to even matter? (Lets consider using something as big as Facebook). If that is the best practice, I am surprised I almost never see it.

Before calling the current code complete, I am thinking the the DB connection code needs to be in at the least a function or a class passing the parameters to it. What would be best practice as far as the connection code. As is, if it were all in its own file complete, you have parameters that could get edited right next to code that shouldn't ever be touched. Additionally, if in a function or class, multiple connections to different DBs could be created if the need was ever there. Also, if in a class or function, should the options be hard coded or passed? What is the best OOP practice for the DB connection code? I realize there is a ton of code out there, but with my lack of experience with OOP, I wouldn't know good from bad at this point.

The if/else is quite clear to me. The && is not. How would I see that the && translates to the same thing as the if since the results are exactly the same with or without it? I am also wondering if the if/else was better to be used for more clarity to those who may read the code. We could also use a Ternary instead of the other two options as well right? (My Preference, although not as readable as the if/else IMO) return $passwordHash ? password_verify($password, $passwordHash) : false;

Why are you returning $passwordHash? The result is the exact same with or without it. return $passwordHash && password_verify($password, $passwordHash);// result: bool true or bool false. return password_verify($password, $passwordHash); // Same exact result: bool true or bool false.

I was aware that it didn't return anything on false. I tried to update my post to ask you about that but the site already locked me out of editing the post. The previous code comment really made sense to me when you put the task to a question, "Is the username and password valid?" which is a yes or no answer, so return Boolean. It got me to re think the process.

Thanks for your input @Jaques1. As of now I have about 12 hours of serious OOP experience against 25+ years of procedural experience. As brief as it was, the code comment was extremely helpful. I cut out at least 50% of the previous code and really got down to a single use class as well as transferring the workload to the class. Here is my revised code. By the way, would the logout function be part of this? What are your thoughts on this revision?

As I get more into OOP it appears I should know and understand unit testing and write tests. Since everything I do is DB driven, nearly all an apps functionality requires a database. From what I read, if you are actually connecting to a DB you are not really doing unit testing, so my question is, how do you do a unit test on a method that gets its data from a DB? Here is the current class I am experimenting with. As I understand it, a unit test is for ONE method. How would I do a unit test for example on the select_user method? class User { /** * @var PDO The connection to the database */ protected $pdo; protected $row; protected $error; protected $password_hash; /** * Construct. * @param PDO $pdo The database connection */ public function __construct($pdo) { $this->pdo = $pdo; } /** * @param $request: $_POST or $_GET * @param $columns: Columns to SELECT */ public function select_user($request, $columns) { $sql = "SELECT "; foreach ($columns AS $field) { $sql .= $field . ', '; } $sql = rtrim($sql, ", "); $sql .= ' FROM users WHERE username = ?'; $stmt = $this->pdo->prepare($sql); $stmt->execute(array( $request )); $row = $stmt->fetch(PDO::FETCH_ASSOC); // No match, Do stuff. if (!$stmt->rowCount()) { $this->error['username'] = 'Bad User Name'; } else { $this->username = $row['username']; $this->password_hash = $row['password']; } } // End function select_user public function verify_password($password) { if (isset($this->password_hash)) { if (password_verify($password, $this->password_hash)) { echo 'Valid User'; // Set $_SESSION['logged_in']; } else { $this->error['password'] = 'Bad Password'; } } } public function check_errors() { if (count($this->error) > 0) { echo 'Errors<br>'; echo $error = implode("<br >\n", $this->error) . "\n"; die; } } // End function check_errors } // End Class User

Does anyone know how facebook or banks do their logged in from new/unknown device routine? Have already googled. Have not found anything definitive yet. In testing, it is not by cookies. I deleted all my cookies and all facebook did was have me re-login. (Notification Settings are set to notify me of new device.)

Current goals accomplished. Anyone see changes that should be made or problems with current code? OOP is making my brain hurt. Done for the day. Not down on what should be public or private yet. From what I do understand I can make everything private. My understanding is that private/public has to do with being able to modify stuff outside the class. Protected? Not even going there at the moment. LATEST CODE: <?php //---------------------------------------------------------------------------- // Database Connection //---------------------------------------------------------------------------- $dbhost = 'localhost'; $dbname = 'meetmarket_development'; $dbuser = 'root'; $dbpass = ''; $charset = 'utf8'; $dsn = "mysql:host=$dbhost;dbname=$dbname;charset=$charset"; $opt = [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, PDO::ATTR_EMULATE_PREPARES => false, ]; $pdo = new PDO($dsn, $dbuser, $dbpass, $opt); //---------------------------------------------------------------------------- // Class User //---------------------------------------------------------------------------- class User { /** * @var PDO The connection to the database */ protected $pdo; protected $row; protected $error; protected $password_hash; /** * Construct. * @param PDO $pdo The database connection */ public function __construct($pdo) { $this->pdo = $pdo; } /** * @param $request: $_POST or $_GET * @param $columns: Columns to SELECT */ public function select_user($request, $columns) { $sql = "SELECT "; foreach ($columns AS $field) { $sql .= $field . ', '; } $sql = rtrim($sql, ", "); $sql .= ' FROM users WHERE username = ?'; $stmt = $this->pdo->prepare($sql); $stmt->execute(array( $request )); $row = $stmt->fetch(PDO::FETCH_ASSOC); // No match, Do stuff. if (!$stmt->rowCount()) { $this->error['username'] = 'Bad User Name'; } else { $this->username = $row['username']; $this->password_hash = $row['password']; } } // End function select_user public function verify_password($password) { if (isset($this->password_hash)) { if (password_verify($password, $this->password_hash)) { echo 'Valid User'; // Set $_SESSION['logged_in']; } else { $this->error['password'] = 'Bad Password'; } } } public function check_errors() { if (count($this->error) > 0) { echo 'Errors<br>'; echo $error = implode("<br >\n", $this->error) . "\n"; die; } } // End function check_errors } // End Class User //---------------------------------------------------------------------------- // //---------------------------------------------------------------------------- /* CURRENT PROGRAM FLOW 1. Compare user submitted username to DB username. If match goto 2. else die with error 2. Username is valid, compare user submitted password to DB hashed password. If match, valid user, else die with error. */ $request = $_POST['username'] = 'myusername'; $password = 'pass'; $user = new User($pdo); $columns = array( 'username', 'password' ); $user->select_user($request, $columns); $user->verify_password($password); $user->check_errors(); ?>

Ok, figured it out. Not sure if it is correct though. <?php //---------------------------------------------------------------------------- // Database Connection //---------------------------------------------------------------------------- $dbhost = 'localhost'; $dbname = 'meetmarket_development'; $dbuser = 'root'; $dbpass = ''; $charset = 'utf8'; $dsn = "mysql:host=$dbhost;dbname=$dbname;charset=$charset"; $opt = [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, PDO::ATTR_EMULATE_PREPARES => false, ]; $pdo = new PDO($dsn, $dbuser, $dbpass, $opt); //---------------------------------------------------------------------------- // Class User //---------------------------------------------------------------------------- class User { /** * @var PDO The connection to the database */ protected $pdo; protected $row; /** * Construct. * @param PDO $pdo The database connection */ public function __construct($pdo) { $this->pdo = $pdo; } /** * @param $request: $_POST or $_GET * @param $columns: Columns to SELECT */ public function select_user($request, $columns) { $sql = "SELECT "; foreach ($columns AS $field) { $sql .= $field . ', '; } $sql = rtrim($sql, ", "); $sql .= ' FROM users WHERE username = ?'; $stmt = $this->pdo->prepare($sql); $stmt->execute(array( $request )); $row = $stmt->fetch(PDO::FETCH_ASSOC); $this -> username = $row['username']; $this -> hash = $row['password']; //return $row; } public function verify_password($password) { echo $password;// User supplied password makes it to here echo $this->hash;// GOT DATA! if (password_verify($password, $this->hash)) { echo 'Valid User';// Set $_SESSION['logged_in']; } else { echo 'Bad'; } } } // End Class User //---------------------------------------------------------------------------- // //---------------------------------------------------------------------------- $request = $_POST['username'] = 'myusername'; $password = 'pass'; $user = new User($pdo); $columns = array( 'username', 'password' ); $user->select_user($request, $columns); $user->verify_password($password); ?>