0

Am Trying to rewrite this code with latest joomla and php for best practice Confused on where to use this function in new code

onchange="showModelVariant(this.value)">

Is the new code correct way of rewriting old code for secure safe way or any better way. Thanks

Am on Joomla 3x series

Old Code

    <?php
    define( '_JEXEC', 1 );
    define('JPATH_BASE', dirname(__FILE__));  
    require_once ( JPATH_BASE .'/includes/defines.php' );
    require_once ( JPATH_BASE .'/includes/framework.php' );
    $mainframe = JFactory::getApplication('site');


$db = JFactory::getDBO();   

if(isset($_GET["n"])){  
    $InsuranceNew=$_GET["n"];
    $sql = "SELECT * from #__newcar_products where state='1' and prod_cat_id='".$InsuranceNew."' order by prod_sorder Asc";
    $db->setQuery($sql);
    $rows = $db->loadObjectList();
    $list='<select name="model" class="list-box-big" id="model"  lang="MUST" title="Model" style="width:245px; height:25px;"  onchange="showModelVariant(this.value)">';
    $list.='<option value="" style="padding-left:10px;" selected>-Select Model-</option>';
    foreach($rows as $row){
        $list.='<option value="'.$row->id .'" style="padding-left:10px;">'.$row->prod_name.'</option>';
    }
    $list.='</select>';
    die($list);
}
    if(isset($_GET["ncvd"])){  
        $NewCarVariantDetail=$_GET["ncvd"];
        $sql = "SELECT * from #__newcar_variants where state='1' and id='".$NewCarVariantDetail."'";
        $db->setQuery($sql);
        $rows = $db->loadObject();
$list='some html code';   
die($list);

New Code

    <?php
        define( '_JEXEC', 1 );
        define('JPATH_BASE', dirname(__FILE__));  
        require_once ( JPATH_BASE .'/includes/defines.php' );
        require_once ( JPATH_BASE .'/includes/framework.php' );
        $mainframe = JFactory::getApplication('site');

         // GET using JInput
        $jinput  = JFactory::getApplication()->input;
        $ModelId = $jinput->get('n', '', 'INT');
        $NewCarVariantDetail = $jinput->get('ncvd', '', 'INT');

        $db = JFactory::getDBO();   
        $query = $db->getQuery(true);
        $query->select($db->qn(array('id','prod_cat_id','prod_name')))
              ->from($db->qn('#__newcar_products'))
              ->where($db->qn('state') . ' = 1')
              ->where($db->qn('prod_cat_id') . ' = ' . (int)$ModelId)
              ->order('prod_sorder ASC');
              $db->setQuery($query);    
            $rows = $db->loadObjectList();
            $list='<select name="model" class="list-box" id="model"  lang="" title="Model">';
            $list.='<option value="" style="padding-left:10px;" selected>-Select Model-</option>';
            foreach($rows as $row){
                $list.='<option value="'.$row->id .'" style="padding-left:10px;">'.$row->prod_name.'</option>';
            }
            $list.='</select>';
            die($list);


$query->select($db->qn(array('id','v_prod_id','v_name')))
              ->from($db->qn('#__newcar_variants'))
              ->where($db->qn('state') . ' = 1')
              ->where($db->qn('v_prod_id') . ' = ' . (int)$NewCarVariantDetail)

              $db->setQuery($query);    
            $rows = $db->loadObjectList();
            $list. = 'some html code';   
die($list);
2
  • ✔️ The use of $jinput->get() with INT parameter to sanitise the incoming data for security and stability.

  • 💡 To maintain a consistent variable naming convention in accordance with Joomla's official recommendation:

    Regular Variables and Class Properties
    Regular variables follow the same conventions as functions.

    This will allow your script to seamlessly blend into the project as a whole and IDEs (e.g. PHPStorm) will be able to intelligently parse the individual words in your variable names (your title case is fine in that regard, but camel case will put all project variables in alignment.

  • ✔️ You are employing Joomla's query builder methods which is not only in alignment Joomla recommendations, but will make your project more portable if you should need to change the db environment that your project exists in or provide your scripting to others.

  • 💡 Your select() method is receiving an array of parameters, which is better than making three separate select() calls, but then the WHERE clause is not constructed in a similar manner. Consider this alternative syntax:

     ->where([
         $db->qn('state') . ' = 1',
         $db->qn('prod_cat_id') . ' = ' . (int)$ModelId
     ]);
    

    This will reduce the total number of where() method calls. I also recommend using square brace syntax for arrays -- this provides an instantly visible distinction between functions and data in scripts that have nested operations in a single statement.

  • 💡 While it is more declarative to state the sort direction (ASC), you may simply write the column name because ASC is the default sorting direction.

  • 💡 It is good practice to keep all of your styling in one place. Ideally, use an external stylesheet instead of inline styling to make your project easier to maintain in the future. On the topic of styling <option> tags, you may like to peruse this page: https://stackoverflow.com/q/22681141/2943403

  • ❌ Most concerning is the use of die($list); and the omission of your conditional check on the input variables. I am not entirely sure what your project requires, but you have certainly lost a portion of logic in your new script. Because die() will kill the script execution at the point that it is called, so your second query block has no chance of being executed. You should revisit your requirements here. If you are building one collection of data in $list, then don't die() / exit() / echo until you are completely finished building the string. (...but if you are building a single string then you could manage to write a single UNION query to return it all in one trip to the db ...again, this part is unclear.)

| improve this answer | |
  • Thanks a lot, but my code somehow is not executing without die statement in. Unsure why but – Ruchika Nov 25 '19 at 7:50
  • Hi the code is only executing with die or echo function. What else can be done, any suggestion – Ruchika Nov 25 '19 at 9:38
  • Echoing or dieing is just fine, but it must be done after $list is fully built. Do you intend to have both query results included in the $list string? I don't have any sample db data, nor do I know your expected result. – mickmackusa Nov 25 '19 at 10:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.