Layout of PHP code

[adammc]

Hi guys,

I have been staring at this for too long and need a fresh set of eyes, if possible?

I want to run the code below above my html page so that if the login is successful I get redirect using 'headerlocation'.

However if the errors array isnt empty i want to echo the errors inside the body of my html page.

I have run into probs when rying to move the block of code cause there is a conflict with the closing bracket of 'check if form was submitted'

<?
// strt session
session_start();

// error reporting
    error_reporting(E_ALL);
    ini_set('display_errors', true);

// include config file
    include "CMS/config.php";


// Check if the form has been submitted.
    if (isset($_POST['submitted'])) { 


// Connect to the database
    include("CMS/script-files/db-connect.php");

    
// Initialise the errors array
    $errors = array(); 
    
// validate the login submission    
    if (empty($_POST['email'])) {
         $errors[] ="Please enter your email address."; 
    }
    
    if (empty($_POST['pass'])) {
        $errors[] ="Please enter your password."; 
    }
    
    $e = $_POST['email'];
    $p = $_POST['pass'];


// If the variables exist, lets continue            
    if ($e && $p) {
    
    
    
// Check if the user id and password combination exist in database
       $sql = "SELECT email, first_name, member_id FROM members WHERE email='$e' AND member_pass=SHA('$p') AND active='Y'";
        $result = mysql_query($sql) 
            or die('Query failed. ' . mysql_error()); 


// if it find a match, continue
       if (mysql_num_rows($result) == 1) {
   
    while($row=mysql_fetch_array($result)) 
   {
   
   
// the user id and password match, lets continue
// lets set the session
      
    $_SESSION['first_name'] = $row['first_name'];
    $_SESSION['members_id'] = $row['member_id'];
    $_SESSION['username'] = $row['email'];        
    
   }
    
      
// if no match was find, report error      
       } else {
       $errors[] ="Sorry, wrong email / password."; 
       }

}    


// If everything went okay and there were no errors, continue.    
    if (empty($errors)) { 


// after login we move to the main page
    header('Location: http://www.domainname/members-area/');

    exit;



// if errors array contains a value        
    } else {
    echo "<table align=\"center\"><tr><td><b>The following error(s) occurred:</b><br /><blockquote>";
    foreach ($errors as $msg) { // Print each error.
    echo "* $msg<br />\n";
    }
    echo '</blockquote></td></tr></table>';
    }
    
}    
    
?>

..html page goes here

hoping to place the echo - error code here '// if errors array contains a value'

[wonshikee]

You do not need "Else", it is redunant.

Because you do if -> header - > exit

If it ever reaches below it, it is ELSE. So ditch the else block, and just stick it below like this

replace: hoping to place the echo - error code here '// if errors array contains a value'

with:

<?php
   if (!empty($error)) {
 echo "<table align=\"center\"><tr><td><b>The following error(s) occurred:</b><br /><blockquote>";

    foreach ($errors as $msg) { // Print each error.

    echo "* $msg<br />\n";

    }

    echo '</blockquote></td></tr></table>';

}
?>

[galen]

1. Include the db file only when necessary. You have it included at a place where it might not get used.

2. use mysql_real_escape_string on variables put into an sql statement

3. im having a hard time finding anything wrong with the code. you're whacky indenting is messing with my head.

[adammc]

Thanx for the reply

You do not need "Else", it is redunant.

But I still need this right:

// If everything went okay and there were no errors, continue.    
    if (empty($errors)) { 

[wonshikee]

But I still need this right:

Yes

[adammc]

I have used the following error reporting:
// error reporting
error_reporting(E_ALL);
ini_set('display_errors', true);

why would I only get a blank white page if there are errors?

[adammc]

1. Include the db file only when necessary. You have it included at a place where it might not get used.

2. use mysql_real_escape_string on variables put into an sql statement

3. im having a hard time finding anything wrong with the code. you're whacky indenting is messing with my head.

1.thanks, great advice
2.Why do theyuse 'sprintf' in their example query?
3. Whaacky! LOL Can you recommend a better way to indent? I'm using Edit pad pro for my php.

[wonshikee]

paste the code here, could be a rogue bracket. and some hosts disable error reporting and clients have no control over it - which sux for development.

[adammc]

I found the missing bracket, the closing brace for this:

// Check if the form has been submitted.
if (isset($_POST['submitted'])) {

With your logic am I meant to close this before I start my html? Cause the error reporting code doesnt appear to be functioning if it doesnt find a user / pass match?

<?
// strt session
session_start();

// error reporting
    error_reporting(E_ALL);
    ini_set('display_errors', true);


// Check if the form has been submitted.
    if (isset($_POST['submitted'])) { 

    
// Initialise the errors array
    $errors = array(); 
    
// validate the login submission    
    if (empty($_POST['email'])) {
         $errors[] ="Please enter your email address."; 
    }
    
    if (empty($_POST['pass'])) {
        $errors[] ="Please enter your password."; 
    }
    
    $e = $_POST['email'];
    $p = $_POST['pass'];


// If the variables exist, lets continue            
    if ($e && $p) {
    

// include config file
    include "CMS/config.php";
    
// Connect to the database
    include("CMS/script-files/db-connect.php");

        
// Check if the user id and password combination exist in database
       $sql = "SELECT email, first_name, member_id FROM members WHERE email='$e' AND member_pass=SHA('$p') AND active='Y'";
        $result = mysql_query($sql) 
            or die('Query failed. ' . mysql_error()); 


// if it find a match, continue
       if (mysql_num_rows($result) == 1) {
   
    while($row=mysql_fetch_array($result)) 
   {
   
   
// the user id and password match, lets continue
// lets set the session
      
    $_SESSION['first_name'] = $row['first_name'];
    $_SESSION['members_id'] = $row['member_id'];
    $_SESSION['username'] = $row['email'];        
    
   }
    
      
// if no match was find, report error      
       } else {
       $errors[] ="Sorry, wrong email / password."; 
       }

}    


// If everything went okay and there were no errors, continue.    
    if (empty($errors)) { 


// after login we move to the main page
    header('Location: http://www.domainname/members-area/');

    exit;


            } // end of if empty errors

}    
?>


html page code begins
....
....
error code here

[wonshikee]

I found the missing bracket, the closing brace for this:

// Check if the form has been submitted.
if (isset($_POST['submitted'])) {

With your logic am I meant to close this before I start my html? Cause the error reporting code doesnt appear to be functioning if it doesnt find a user / pass match?

Yes, that if block should be closed. That is what you were looking for no?

if (/*submitted*/) {
    if (/*no error*/) header()
}

if (/*error*/) #display error

# HTML 

Also you should seriously consider organizing your code format so it is easily readable, I dunno edit pad pro, but you can get something free like notepad++ and indent your code properly so that each block's domain is readily visible.

How you indent is strictly a personal preference, but I personally do a hybrid of both.

For if/elseif/else I do

if () {
} else {
} 

So I can see their flow, which if the else belongs to.
and for everything else i do

function()
{
    $a;
} 

Because with 4 space indents, the blocks really stand out.

[adammc]

Looking at the php code in my last post..... Can you see any reason why the error reporting code below when placed into the html wont return a result even when there is a user pass mismatch?

<?php

// if errors array isnt empty
if (!empty($error)) { 

    echo "<table align=\"center\"><tr><td><b>The following error(s) occurred:</b><br /><blockquote>";      
    foreach ($errors as $msg) { 
// Print each error.      

    echo "* $msg<br />\n";      }      
    echo '</blockquote></td></tr></table>';  
    } 
?>

[adammc]

nevermind, i just removed the check below and it works fine

if (!empty($error)) { 

I cant see a prob in doing this as the only reason this code (html page) gets executed is if there 'is' an error.

Thank you so much for your help, much appreciated

[wonshikee]

The variable is $errors, not $error. My fault - I never use plural variable names no matter what for that reason, always forget if it had an s or not.