Coding Standards

Client Joined: Oct 07, 2003 Posts: 735 Location: Ohio

Using Evaders99 excellent page here.

http://evaders.swrebellion.com/modules.php?name=Index&readme=1&page=coding

I am retooling a module I wrote trying to bring it up to current patched standards.

Can a file meet more than one of these conditions?

Code:

define('MODULE_FILE', true);


if (!defined('MODULE_FILE')) {


    die ("You can't access this file directly...");


}





define('ADMIN_FILE', true);


if (!defined('ADMIN_FILE')) {


   die ("Access Denied");


} 





define('NUKE_FILE', true);


if (!defined('NUKE_FILE')) {


   die ("Inclusion of Mainfile undefined");


}

Can somebody also break down the process how an admin script in the

modules/XXXXX/admin/whatever.php

is accessed? If it cannot be accessed then why would it default me to the public index.php of my module.

I am going in circles here.

registered · Posted: Wed Feb 15, 2006 3:53 pm

admin files are driven from admin.php
module files are driven from modules.php
I don't think there is any reason for them to both be used

Well basically admin.php defines 'ADMIN_FILE' - no other script should define it
Thus when you go directly to the admin/whatever.php file, it looks for this constant.
It will only be accessible throught admin.php?op= ... that is dependent on the case file to link the op to the actual file name

Posted: Wed Feb 15, 2006 5:08 pm

Quote:

Warning: main(admin/addsoldier.php): failed to open stream: No such file or directory in /home/xxxxx/public_html/milpacs/modules/MILPACS/admin/index.php on line 52

My link file:

Code:

if ($radminsuper==1) {


    adminmenu("admin.php?op=milpacs", _MILPACS_MENU, "milpacs.png");


}

My case file:

Code:

switch($op) {


       case "milpacs":


      case "AddMedal":


      case "AddRank":


      case "AddSoldier":


      case "AddWeapon":


      case "AddServiceRecord":


      case "AddMedalRecord":


      case "AddDrillReport":


      case "AddUnit":


      case "AddSubUnit":


      case "AddAdminUnit":


      case "EditMedal":


      case "EditRank":   


      case "EditSoldier":


      case "EditWeapon":


      case "EditServiceRecord":


      case "EditMedalRecord":


      case "EditDrillReport":


      case "EditSubUnit":   


      case "EditUnit":


      case "EditWar":   


      case "EditService":


      case "DelServiceRecord":   


      case "DelMedalRecord":      


      case "DelSubUnit":      


      case "DelAdminUnit":


      case "ServiceRecord":


      case "MedalRecord":      


include("modules/$module_name/admin/index.php");


   break;


}

My MILPACS/admin/index.php comes up fine.

But when I want to go to addsoldier.php like here:

Code:

<p><a href="admin.php?op=AddSoldier">Add a new troop</a></p>

It should send me to my addsoldier page

Code:

define('ADMIN_FILE',true);


if ( !defined('ADMIN_FILE')) {


    Header("Location: ../../admin.php");


    die("Illegal File Access");


}


etc


etc





<form name="addsoldier" action="admin.php?op=AddSoldier" method="POST">





etc


etc

Now instead of sending me to my public index.php it sends me to my MILPACS/admin/index.php, which is a good thing right... Very Happy

registered · Posted: Wed Feb 15, 2006 8:11 pm

Donovan,

The following in your addsoldier.php script:

define('ADMIN_FILE',true);
if ( !defined('ADMIN_FILE')) {
Header("Location: ../../admin.php");
die("Illegal File Access");
}

Should really read:

if (!defined('ADMIN_FILE')) {
die ("Access Denied");
}

You really do NOT want a back attempt to route back to your admin index file. You want it to die right there. There is NO excuse for someone to try and access this file directly. It is ALWAYS a hack attempt!

Posted: Wed Feb 15, 2006 9:28 pm

So then in my admin/index.php file I should have this

Code:

</td>


      <td align="center" width="10%"><a href="admin.php?op=EditSoldier&id=<?php echo $id ?>">Edit</a><p>


      </td>


      <td align="center" width="10%"><a href="admin.php?op=ServiceRecord&id=<?php echo $id ?>">Edit</a><p>


      </td>


      <td align="center" width="10%"><a href="admin.php?op=MedalRecord&id=<?php echo $id ?>">Edit</a><p>


      </td>

Instead of this

Code:

</td>


      <td align="center" width="10%"><a href="modules.php?name=MILPACS&op=EditSoldier&id=<?php echo $id ?>">Edit</a><p>


      </td>


      <td align="center" width="10%"><a href="modules.php?name=MILPACS&op=ServiceRecord&id=<?php echo $id ?>">Edit</a><p>


      </td>


      <td align="center" width="10%"><a href="modules.php?name=MILPACS&op=MedalRecord&id=<?php echo $id ?>">Edit</a><p>


      </td>

Posted: Thu Feb 16, 2006 7:03 am

Yes. And, your case.php needs to have the same values for the $op variable and the switch in your admin/index.php for the $op variable needs to be the same and either call out a function to execute (traditional nuke approach) or include the right file for that function (this is how I have done it).

Posted: Thu Feb 16, 2006 10:42 am

Why on earth would I be getting this error?

Quote:

Warning: main(modules/admin/admin/addsoldier.php): failed to open stream:

My admin path is displayed twice!!!

That is not how it looks anywhere in my code.

index.php

Code:




switch($op) {   


      case "milpacs":milpacs();break;


      case "AddMedal":include("modules/$module_name/admin/addmedal.php");break;


      case "AddRank":include("modules/$module_name/admin/addrank.php");break;


      case "AddSoldier":include("modules/$module_name/admin/addsoldier.php");break;

case.php

Code:




switch($op) {


       case "milpacs":


      case "AddMedal":


      case "AddRank":


      case "AddSoldier":


                                etc


                                etc


include("modules/$module_name/admin/index.php");


   break;


}

Posted: Thu Feb 16, 2006 12:20 pm

Something in there must not be setting $module_name correclty

Posted: Thu Feb 16, 2006 7:27 pm

Donovan, do you have something similar to this towards the top of your index.php script?

require_once("mainfile.php");
$module_name = basename(dirname(__FILE__));
get_lang($module_name);

This is how it is typically done or hard-coded.

Posted: Thu Feb 16, 2006 7:46 pm

Yes I do. I had to hard code in like this since $module_name was getting stepped on.

Code:




case "AddMedal":include("modules/MILPACS/admin/addmedal.php");break;


      case "AddRank":include("modules/MILPACS/admin/addrank.php");break;


      case "AddSoldier":include("modules/MILPACS/admin/addsoldier.php");break;


      case "AddWeapon":include("modules/MILPACS/admin/addweapon.php");break;

Now everything comes up. I'll keep digging though and try and find out why.

Last question.

Since I am now using admin.php?op=.....

Should I have the form proccessing like so:

Code:

<form name="editsoldier" action="admin.php?op=EditSoldier" method="post">

My page is not processing with this when I click submit.

All I get is a white screen with the url as:

Quote:

http://milpacs.3rd-infantry-division.net/admin.php?op=EditSoldier

(if you notice I am working in a subdomain off my main site)

Posted: Thu Feb 16, 2006 7:58 pm

Here is one of my pages so you can see how I am doing things. I pretty much followed this basic method throughout my module.

Code:

/*********************************************************/


/* Add Medal                                             */


/*********************************************************/


if ( !defined('ADMIN_FILE')) {


   die("Illegal File Access");


}





define('INDEX_FILE', false);


$index = 0;


include_once("header.php"); 


global $module_name, $db, $prefix;





// Load Medal image


   $urlofimages="$DOCUMENT_ROOT/modules/MILPACS/images/medals/";


   $medalselecthtml = "<select name=\"award_image\">";


   $medalselecthtml .= "<option value=\"\">Select Medal Image</option>";


   if ($handle=@opendir($urlofimages)) { 


      while ($imgfile = readdir($handle)) {


         if ($imgfile != "." && $imgfile != ".." && $imgfile != "" && $imgfile != "index.html" && $imgfile != "WS_FTP.LOG" && $imgfile != "Thumbs.db") {


            if ($imgfile==$award_image) {


               $medalselecthtml .= "<option value =\"$imgfile\" selected>$imgfile</option>";


            } else {


               $medalselecthtml .= "<option value =\"$imgfile\" >$imgfile</option>";


            }


         }


      }


      @closedir($handle);


   }


   $medalselecthtml .= "</select></td>";


   





if ($op == "NewMedal") {


      // Validations go here


      // If all validations passed, save and exit, otherwise, redisplay with errors


      $award_name = $_POST['award_name'];


      $award_description = addslashes($_POST['award_description']);      


      $award_image = $_POST['award_image'];


      $award_class = $_POST['award_class'];


      //Insert the values into the database 


   $sql = "INSERT INTO " . $prefix . "_milpacs_awards (award_id, award_name, award_image, award_description, award_class)". "VALUES ('NULL','$award_name','$award_image','$award_description', '$award_class')";


      $result = $db->sql_query($sql);


      if (!$result) {


      echo("<p>Error performing query: " . mysql_error() . "</p>");


      }


}      





OpenTable();


echo "<p><a href=\"admin.php?op=milpacs\">Return to Main Administration</a></p>";


?>





<form name="addmedal" action="admin.php?op=AddMedal" method="POST">   


 <table width="100%" border="2" cellpadding="2" align ="center" cellspacing="0" style="border-collapse: collapse;" bgcolor="#000000" bordercolor="#111111">


  <tr>


           <td align="center" bgcolor="#777777">


              <b><font color="#000000"><?php echo "._MILPACS_TAG." ?> Medal <?php echo $award_name ?></font></b>


           </td>


         <tr>


           <td align="left" bgcolor="#666633">


            <b><font color="#000000">Medal Information</font></b>


           </td>


        </tr>


  <table border=0 width='100%' cellpadding='3'><tr><th width='20%'>Medal Image</th><th width='30%'>Name of Medal</th><th width='20%'><b>Medal Details</b></th><th width='10%'><b>Medal Class</b></th></tr>  


      <tr>


         <td align="center" size="20" bgcolor="#999999"> <?php echo $medalselecthtml ?>


         </td>


         <td align="center" bgcolor="#999999"><input type="text" size="40" name="award_name">


         </td>


         <td align="center" bgcolor="#999999"><textarea name="award_description" cols="30" colspan="1" rows="3"> <?php echo$award_description ?></textarea>


         </td>


         <td align="center" bgcolor="#999999">


         <select NAME="award_class">


               <option VALUE="SB">Skill Badge


               <option VALUE="UC">Unit Citiation


            <option VALUE="IM">Individual Medal


          </select>         


         </td>


         </tr>


</table>


<br>


<br>


<hr>


<input type="hidden" name="op" value="NewMedal">


<input type="hidden" name="award_id" value="<?php echo $award_id ?>"/>


<input type="submit" align="center" name="Submit" value="Add"/>


</form>


<?php


CloseTable();


include("footer.php");


?>

Posted: Thu Feb 16, 2006 10:28 pm

Donovan,

I would do this differently:

Code:




<form name="editsoldier" action="admin.php" method="post">


<input type="hidden" name="op" value="EditSoldier">


etc...

Actually, I just saw in your followup example that you are setting "op" to "NewMedal" down towards the bottom. Your action would only be admin.php. Well, actually, if you are using 7.6, you should really should not hard-code "admin.php", but that is your choice to make and you are probably not going to distribute this, so who cares... Smile

By the way, if you were using "GET" instead of "POST", this may have worked how you had it, but I would stick with POST. It keeps your variable values out of your URLs.

Posted: Fri Feb 17, 2006 3:58 pm

action="admin.php"

or

action="admin.php?op=EditSoldier"

gives me a blank page.

Posted: Sat Feb 18, 2006 8:30 am

Donovan,

Quote:

Actually, I just saw in your followup example that you are setting "op" to "NewMedal" down towards the bottom

You have to be careful not to define your same input variables multiple times.

I have also have a feeling you have also introduced a parse error. Do you have your display errors turned on in your config.php? Do that for a short time to get the error message and then turn it off.

What are you using for your sample script? You may want to look at something like modules/Content/admin/index.php and model your script after it. Or you can look at mine, however, I have done some things with my config files and functions a little differently than most, but it keeps things well organized and much easier to maintain go forward (hence why I re-wrote most of the app for version 1.3).

Posted: Sat Feb 18, 2006 10:44 am

Thanks for your help. I am making some good head way on this.

One question.

The op=EditSoldier is used in the switch in my admin/index.php to send me from there to my editsoldier.php file.

example:

Code:

case "EditSoldier":include("modules/$module_name/admin/editsoldier.php");break;

Internally in my editsoldier.php file I was going to use op=SaveSoldier but it doesn't work. I get the blank page. I don't have SaveSoldier defined anywhere else but here.

If I use this instead:

Code:

 if ($op == "EditSoldier") {





$id = intval($_POST['id']);


   $rank_id = intval($rank_id);


   $unit_id = intval($unit_id);


etc


etc...





// If not saving, load values from database


if ($op != "EditSoldier") {


   $id = intval($_GET['id']);


$result = $db->sql_query("SELECT * FROM " . $prefix . "_milpacs_members..... etc

I don't get the blank page, however it never pulls the members from the databse cause the op is defined as EditSoldier.

I don't understand why SaveSoldier wont work, or at least the page will not display. I understand why EditSoldier doesn't work, but at least the page processes even though nothing happens.

Am I confusing you? Laughing

I am using this for now

Code:

<form name="editsoldier" action="admin.php" method="post">

until I implement

Code:

."<form action=\"".$admin_file.".php\" method=\"post\">"

Posted: Sat Feb 18, 2006 12:57 pm

Took your advice and studied the content module.

I made the following changes to editsoldier:

Code:

// Load values from database





   $id = intval($_GET['id']);


   $result = $db->sql_query("SELECT * FROM " . $prefix . "_milpacs_members mm JOIN " . $prefix . "_milpacs_units mu JOIN " . $prefix . "_milpacs_weapons mw WHERE mm.uniqueid ='$id' AND mm.unit_id = mu.unit_id AND mm.weapon_id = mw.weapon_id");


   $info = $db->sql_fetchrow($result);


   if (!$result) {


      echo("<p>Error performing query: " . mysql_error() . "</p>");


      exit();


   } else {


      $soldierName = $info[u_name];


      $nukeusername = $info[nuke_username];


      $uniform = $info[uniform];


      $rank_id = $info[rank_id];


      $flag = $info[flag];

etc

It doesn't check if ($op != "savesoldier") but just loads the values from the table.

At the end I have:

Code:

<input type="hidden" name="op" value="SaveSoldier"/>


<input type="hidden" name="id" value="<?php echo $id ?>"/>

Which is now a function and is passed ($id):

Code:

function SaveSoldier($id) {


   // Validations go here


   // If all validations passed, save and exit, otherwise, redisplay with errors


   $id = intval($_POST['id']);


   $rank_id = intval($rank_id);


   $unit_id = intval($unit_id);


   $weapon_id = intval($weapon_id);


   $subunit_id = intval($subunit_id);


   $adminunits;


   $num_of_adminunits = sizeof($_POST[admin_unit_id]);


   $k = 0;


   for ($i=0; $i < $num_of_adminunits; $i++) {


      $ifDuplicated = false;


      $test_for_duplication = explode(",",$adminunits);


      if ($_POST[admin_unit_id][$i]) {


         for ($j=0; $j < sizeof($test_for_duplication); $j++) {


            if ($_POST[admin_unit_id][$i] == $test_for_duplication[$j])


               $ifDuplicated = true;


         }


         if (!$ifDuplicated) {


            if ($k > 0)


               $adminunits .= ",";


            $adminunits .= $_POST[admin_unit_id][$i];


            $k++;


         }


      }


   }


//      Order in increasing numerical order


   $admin_array = explode(",",$adminunits);


   sort($admin_array);


   $adminunits = "";


   for ($i=0; $i <= sizeof($admin_array); $i++) {


      if ($admin_array[$i] != "")


         $adminunits .= $admin_array[$i] . ",";


   }


//   $email = addslashes($email);


   $bio = addslashes($bio);


   $sql = "UPDATE " . $prefix . "_milpacs_members set


      uniform = '$uniform',


      rank_id = $rank_id,


      flag = '$flag',


      u_name = '$soldierName',


      nuke_username =   '$nukeusername',


      location = '$location',


      status = '$status',


      p_mos = '$p_mos',


      unit_id = $unit_id,


      subunit_id = $subunit_id,


      adminunits = '$adminunits',


      reports = '$reports',


      position = '$position',


      weapon_id = $weapon_id,


      enlistment_dt = '$enlistment_dt',


      promotion_dt = '$promotion_dt',


      icq = '$icq',


      email = '$email',


      bio = '$bio'


      WHERE uniqueid ='$id'";


   $update = $db->sql_query($sql);


OpenTable();


echo "<b>" . _EDITSOLDIERDONE . "</b><br>";


echo "<p><a href=\"admin.php?op=milpacs\">Return to Main Administration</a></p>";


CloseTable();


}


?>

Same result, all I get is a blank white page.

I have $display_errors = true; in my config but no errors are shown.

Posted: Sat Feb 18, 2006 10:02 pm

A blank white page is a parse error somewhere. You are going to have to do through the code with a "fine tooth comb" and find it.

Posted: Sun Feb 19, 2006 12:06 am

Well if I have $display_errors = true; in my config, then why do I still see a blank white page?

Posted: Sun Feb 19, 2006 7:42 am

Well, you say that your case and link statements are "correct", so not sure what else to tell you. If you assign the op variable to a value and that value is not in your case.php, you will get the blank page for sure and that would NOT be a parse error.

I am just accepting the assumption that you made that your case statement is correct.

Regarding display_errors = true, I may have stated it wrongly in an earlier post. If you have a parse error, PHP just dies (my experience, not from some manual reading). However, if PHP is able to trap the error, this, I believe, is where display_errors comes in.

My appologies for not reading your not several pages up carefully enough that you were getting a blank page. Sometimes people say a generic statement of "my page doesn't come up", well that can mean anything from a blank white page, to the page is only partially completed (usually a php trapped error but display_errors is false), to the web server times out, to ....... Get the point?

You have review your code carefully to find the bad PHP statement(s).

Posted: Sun Feb 19, 2006 11:08 am

montego wrote:

If you assign the op variable to a value and that value is not in your case.php, you will get the blank page for sure and that would NOT be a parse error.

But could I have an op=SaveSoldier in my editsoldier.php file that calls a function located on the same page? SaveSoldier is not in my case since it is not used on my admin/index.php page. SaveSoldier is only used on my editsoldier page.

At the bottom of my form on editsoldier:

Code:

<input type="hidden" name="op" value="SaveSoldier">

My function located in editsoldier.php:

Code:

function SaveSoldier($id) {


   // Validations go here


   // If all validations passed, save and exit, otherwise, redisplay with errors


   $id = intval($_POST['id']);


   $rank_id = intval($rank_id);


   $unit_id = intval($unit_id);


   $weapon_id = intval($weapon_id);


   $subunit_id = intval($subunit_id);


   $adminunits;


   $num_of_adminunits = sizeof($_POST[admin_unit_id]);


   $k = 0;


   for ($i=0; $i < $num_of_adminunits; $i++) {


      $ifDuplicated = false;


      $test_for_duplication = explode(",",$adminunits);


      if ($_POST[admin_unit_id][$i]) {


         for ($j=0; $j < sizeof($test_for_duplication); $j++) {


            if ($_POST[admin_unit_id][$i] == $test_for_duplication[$j])


               $ifDuplicated = true;


         }


         if (!$ifDuplicated) {


            if ($k > 0)


               $adminunits .= ",";


            $adminunits .= $_POST[admin_unit_id][$i];


            $k++;


         }


      }


   }


//      Order in increasing numerical order


   $admin_array = explode(",",$adminunits);


   sort($admin_array);


   $adminunits = "";


   for ($i=0; $i <= sizeof($admin_array); $i++) {


      if ($admin_array[$i] != "")


         $adminunits .= $admin_array[$i] . ",";


   }


//   $email = addslashes($email);


   $bio = addslashes($bio);


   $sql = "UPDATE " . $prefix . "_milpacs_members set


      uniform = '$uniform',


      rank_id = $rank_id,


      flag = '$flag',


      u_name = '$soldierName',


      nuke_username =   '$nukeusername',


      location = '$location',


      status = '$status',


      p_mos = '$p_mos',


      unit_id = $unit_id,


      subunit_id = $subunit_id,


      adminunits = '$adminunits',


      reports = '$reports',


      position = '$position',


      weapon_id = $weapon_id,


      enlistment_dt = '$enlistment_dt',


      promotion_dt = '$promotion_dt',


      icq = '$icq',


      email = '$email',


      bio = '$bio'


      WHERE uniqueid ='$id'";


   $update = $db->sql_query($sql);


OpenTable();


echo "<b>" . _EDITSOLDIERDONE . "</b><br>";


echo "<p><a href=\"admin.php?op=milpacs\">Return to Main Administration</a></p>";


CloseTable();


}


?>

Posted: Sun Feb 19, 2006 10:22 pm

Donovan, ok, you win. I cannot follow all the piece-meal posts of code because you have a parse error somewhere in the page and/or issue with case.php. If you have given this a thorough review and you still cannot find it, please send me the following: your module admin index.php, case.php, links.php, editsoldier.php and any other included scripts for this page. I just cannot possibly find it when everything is so scattered in various posts.

Send to montego (( at )) montegoscripts __ Dot __ com.

Posted: Mon Feb 20, 2006 8:04 am

Donovan, now that I have gotten some sleep, I must appologize. The tone in my previous post was not intentional; it could have come off wrong.

Seriously, if you would like some help looking these over, send over your complete module package and I can spend some time looking at it over the next couple of days. Just so you know, I only get an hour or so in the mornings to do this stuff, so please be patient with me.