[not a bug] Using array key as loop counter - bad idea

[durangod]

There are numerious places in the software where you use $num which is the element key as a loop counter. This is a very bad idea, especially for any kind of sorting whatsoever. Although this is not a bug in the sense that it is broken, it is a bug in the sense that it is very very wrong to do it this way for many reasons.

You can use a search utility and search for "div by" or "$num" to find most of the places this needs to be fixed.

 

here is an example of what i mean:

 

in portal/domainchecker.tpl

 

   {foreach key=num item=listtld from=$tldslist}                             
       <td align="left">
       <input type="checkbox" name="tlds[]" value="{$listtld}"{if in_array($listtld,$tlds)} checked{/if}>
       {$listtld}
       </td>
       {if $num % 5 == 0}</tr><tr>{/if}
       {/foreach}

 

by using the num as a loop counter for the rows you totaly limit the ability and functionality of the script and any hope of any kind of real sort flexibility or row display flexibility. As well as it is not standard practice to do so.

 

here is a better more standardized way... use a loop counter..

 

smarty already has the functionality to and actual function to do a counter, you just have to install the function.

 

This is a version of the counter function from one of my other smarty scripts, so WHMCS needs to check it to be sure it is totally up to date. I believe it is from 2.6.26 although it may also be a custom function as well.

 

the name of the file is function.counter.php

and it goes inside includes/classes/Smarty/plugins

 

<?php
/**
* Smarty plugin
* @package Smarty
* @subpackage plugins
*/


/**
* Smarty {counter} function plugin
*
* Type:     function<br>
* Name:     counter<br>
* Purpose:  print out a counter value
* @author Monte Ohrt <monte at ohrt dot com>
* @link http://smarty.php.net/manual/en/language.function.counter.php {counter}
*       (Smarty online manual)
* @param array parameters
* @param Smarty
* @return string|null
*/
function smarty_function_counter($params, &$smarty)
{
   static $counters = array();

   $name = (isset($params['name'])) ? $params['name'] : 'default';
   if (!isset($counters[$name])) {
       $counters[$name] = array(
           'start'=>1,
           'skip'=>1,
           'direction'=>'up',
           'count'=>1
           );
   }
   $counter =& $counters[$name];

   if (isset($params['start'])) {
       $counter['start'] = $counter['count'] = (int)$params['start'];
   }

   if (!empty($params['assign'])) {
       $counter['assign'] = $params['assign'];
   }

   if (isset($counter['assign'])) {
       $smarty->assign($counter['assign'], $counter['count']);
   }

   if (isset($params['print'])) {
       $print = (bool)$params['print'];
   } else {
       $print = empty($counter['assign']);
   }

   if ($print) {
       $retval = $counter['count'];
   } else {
       $retval = null;
   }

   if (isset($params['skip'])) {
       $counter['skip'] = $params['skip'];
   }

   if (isset($params['direction'])) {
       $counter['direction'] = $params['direction'];
   }

   if ($counter['direction'] == "down")
       $counter['count'] -= $counter['skip'];
   else
       $counter['count'] += $counter['skip'];

   return $retval;

}

/* vim: set expandtab: */

?>

 

once you have this function installed in the plugins folder then you can use {counter} by default most of the attributes are fine. However you will need to assign a var to compare and also set print to false

 

and you do it like this:

we will use the same code example from the domainchecker above but this time it will use the counter as the loop so we can sort it properly and display it properly in rows. You can also see that i added the sorttld modifer as well. Which you can find information about that here. http://forum.whmcs.com/showthread.php?90071-sorting-tlds-mod&p=379494#post379494

 

      {foreach key=num item=listtld from=$tldslist|@sorttld}
         {counter assign=loopctr print=false}                        
       <td align="left">
       <input type="checkbox" name="tlds[]" value="{$listtld}"{if in_array($listtld,$tlds)} checked{/if}/>
       {$listtld}
       </td>
       {if $loopctr % 5 == 0}</tr><tr>{/if}
       {/foreach}

 

that is the right way to do it..

 

setting var loopctr allows us to compare that var, which will be consistantly low to high the way we have it set so it is much more reliable as a counter than the $num (key value) which jumps all over the place if you sort in any way. And setting print to false means the counter will not be displayed (parsed) to the page.

 

you can see i also am adding the close / to all the inputs i find, which is also something that needs to be done but that is another matter.

 

and now with this new way, the upper portion of the domain checker can be sorted properly.

 

What this solves: If we stuck with the old way using the key id we could never sort this because if we used sorttld modifier the numbers would be all over the place and so would the rows because you would have something like 126 254 287 14 1 3 6 54 87 so that is why we never should use the key id as a loop counter to set display rows.

 

I feel all of the places that use $num (key)as a loop counter should be replaced with the smarty or other type of valid seperator counter var.

 

I hope this explains it... thanks

 

you can find the attributes for counter here http://www.smarty.net/docsv2/en/language.function.counter.tpl

Edited June 14, 2014 by durangod

[WHMCS Nate]

Durango Dave,

 

Thanks for providing the feedback here. There are a few different things which are brought up here I wanted to note:

 

1) The smarty version - this is something that has been scheduled to be upgraded to smarty3, I expect that the next beta release of our future major version (was that vague enough?). We will also be using composer to maintain a number of our third party dependencies which should aid with pushing regular updates to them faster.

 

2) Using array keys as a counter - as you noted this not actually a direct software defect, it is something that could be done better. I have raised this with the development team and we will consider using the counter function in smarty for future templates. However as we work on the new client area we have shifted a lot of our filtering and sorting out of smarty and into JS on the client side. As such the objection that using the array keys makes sorting harder is void.

 

In the future when reporting issues like this, you may find the technical support area a better location. We try to reserve this section for defect reports.

 

Thanks again for your report, have a great day.

 

Nate C