10. Real Code Cases

10.1. Introduction

All the examples in this section are real code, extracted from major PHP applications.

10.2. Examples

10.2.1. Adding Zero

10.2.1.1. Thelia

Adding Zero, in core/lib/Thelia/Model/Map/ProfileResourceTableMap.php:250.

This return statement is doing quite a lot, including a buried ‘0 + $offset’. This call is probably an echo to ‘1 + $offset’, which is a little later in the expression.

return serialize(array((string) $row[TableMap::TYPE_NUM == $indexType ? 0 + $offset : static::translateFieldName('ProfileId', TableMap::TYPE_PHPNAME, $indexType)], (string) $row[TableMap::TYPE_NUM == $indexType ? 1 + $offset : static::translateFieldName('ResourceId', TableMap::TYPE_PHPNAME, $indexType)]));

10.2.1.2. OpenEMR

Adding Zero, in interface/forms/fee_sheet/new.php:466:534.

$main_provid is filtered as an integer. $main_supid is then filtered twice : one with the sufficent (int) and then, added with 0.

if (!$alertmsg && ($_POST['bn_save'] || $_POST['bn_save_close'] || $_POST['bn_save_stay'])) {
    $main_provid = 0 + $_POST['ProviderID'];
    $main_supid  = 0 + (int)$_POST['SupervisorID'];
    //.....

10.2.2. Ambiguous Array Index

10.2.2.1. PrestaShop

Ambiguous Array Index, in src/PrestaShopBundle/Install/Install.php:532.

Null, as a key, is actually the empty string.

$list = array(
            'products' => _PS_PROD_IMG_DIR_,
            'categories' => _PS_CAT_IMG_DIR_,
            'manufacturers' => _PS_MANU_IMG_DIR_,
            'suppliers' => _PS_SUPP_IMG_DIR_,
            'stores' => _PS_STORE_IMG_DIR_,
            null => _PS_IMG_DIR_.'l/', // Little trick to copy images in img/l/ path with all types
        );

10.2.2.2. Mautic

Ambiguous Array Index, in app/bundles/CoreBundle/Entity/CommonRepository.php:314.

True is turned into 1 (integer), and false is turned into 0 (integer).

foreach ($metadata->getAssociationMappings() as $field => $association) {
                    if (in_array($association['type'], [ClassMetadataInfo::ONE_TO_ONE, ClassMetadataInfo::MANY_TO_ONE])) {
                        $baseCols[true][$entityClass][]  = $association['joinColumns'][0]['name'];
                        $baseCols[false][$entityClass][] = $field;
                    }
                }

10.2.3. error_reporting() With Integers

10.2.3.1. SugarCrm

error_reporting() With Integers, in modules/UpgradeWizard/silentUpgrade_step1.php:436.

This only displays E_ERROR, the highest level of error reporting. It should be checked, as it happens in the ‘silentUpgrade’ script.

ini_set('error_reporting', 1);

10.2.4. Eval() Usage

10.2.4.1. XOOPS

Eval() Usage, in htdocs/modules/system/class/block.php:266.

eval() execute code that was arbitrarily stored in $this, in one of the properties. Then, it is sent to output, but collected before reaching the browser, and put again in $content. May be the echo/ob_get_contents() could have been skipped.

ob_start();
                    echo eval($this->getVar('content', 'n'));
                    $content = ob_get_contents();
                    ob_end_clean();

10.2.4.2. Mautic

Eval() Usage, in app/bundles/InstallBundle/Configurator/Step/CheckStep.php:238.

create_function() is actually an eval() in disguise : replace it with a closure for code modernization

create_function('$cfgValue', 'return $cfgValue > 100;')

10.2.5. Exit() Usage

10.2.5.1. Traq

Exit() Usage, in src/Controllers/attachments.php:75.

This acts as a view. The final ‘exit’ is meant to ensure that no other piece of data is emitted, potentially polluting the view. This also prevent any code cleaning to happen.

/**
     * View attachment page
     *
     * @param integer $attachment_id
     */
    public function action_view($attachment_id)
    {
        // Don't try to load a view
        $this->render['view'] = false;

        header(Content-type: {$this->attachment->type});
        $content_type = explode('/', $this->attachment->type);

        // Check what type of file we're dealing with.
        if($content_type[0] == 'text' or $content_type[0] == 'image') {
            // If the mime-type is text, we can just display it
            // as plain text. I hate having to download files.
            if ($content_type[0] == 'text') {
                header(Content-type: text/plain);
            }
            header("Content-Disposition: filename=\"{$this->attachment->name}\"");
        }
        // Anything else should be downloaded
        else {
            header("Content-Disposition: attachment; filename=\"{$this->attachment->name}\"");
        }

        // Decode the contents and display it
        print(base64_decode($this->attachment->contents));
        exit;
    }

10.2.5.2. ThinkPHP

Exit() Usage, in ThinkPHP/Library/Vendor/EaseTemplate/template.core.php:60.

Here, exit is used as a rudimentary error management. When the version is not correctly provided via EaseTemplateVer, the application stop totally.

$this->version              = (trim($_GET['EaseTemplateVer']))?die('Ease Templae E3!'):'';

10.2.6. Multiply By One

10.2.6.1. SugarCrm

Multiply By One, in SugarCE-Full-6.5.26/modules/Relationships/views/view.editfields.php:74.

Here, ‘$count % 1’ is always true, after the first loop of the foreach. There is no need for % usage.

$count = 0;
        foreach($this->fields as $def)
        {
            if (!empty($def['relationship_field'])) {
                $label = !empty($def['vname']) ? $def['vname'] : $def['name'];
                echo <td> . translate($label, $this->module) . :</td>
                   . <td><input id='{$def['name']}' name='{$def['name']}'>  ;

                if ($count%1)
                    echo </tr><tr>;
                $count++;
            }
        }
        echo </tr></table></form>;

10.2.6.2. Edusoho

Multiply By One, in wp-admin/includes/misc.php:74.

1 is useless here, since 24 * 3600 is already an integer. And, of course, a day is not 24 * 3600… at least every day.

'yesterdayStart' => date('Y-m-d', strtotime(date('Y-m-d', time())) - 1 * 24 * 3600),

10.2.7. Not Not

10.2.7.1. Cleverstyle

Not Not, in modules/OAuth2/OAuth2.php:190.

This double-call returns $results as a boolean, preventing a spill of data to the calling method. The (bool) operator would be clearer here.

$result = $this->db_prime()->q(
                    [
                            DELETE FROM `[prefix]oauth2_clients`
                            WHERE `id` = '%s',
                            DELETE FROM `[prefix]oauth2_clients_grant_access`
                            WHERE `id`      = '%s',
                            DELETE FROM `[prefix]oauth2_clients_sessions`
                            WHERE `id`      = '%s'
                    ],
                    $id
            );
            unset($this->cache->{'/'});
            return !!$result;

10.2.7.2. Tine20

Not Not, in tine20/Calendar/Controller/MSEventFacade.php:392.

It seems that !! is almost superfluous, as a property called ‘is_deleted’ should already be a boolean.

foreach ($exceptions as $exception) {
                $exception->assertAttendee($this->getCalendarUser());
                $this->_prepareException($savedEvent, $exception);
                $this->_preserveMetaData($savedEvent, $exception, true);
                $this->_eventController->createRecurException($exception, !!$exception->is_deleted);
            }

10.2.8. include_once() Usage

10.2.8.1. XOOPS

include_once() Usage, in /htdocs/xoops_lib/modules/protector/admin/center.php:5.

Loading() classes should be down with autoload(). autload() may be build in several distinct functions, using spl_autoload_register().

require_once dirname(__DIR__) . 'class/gtickets.php'

10.2.8.2. Tikiwiki

include_once() Usage, in tiki-mytiki_shared.php :140.

Turn the code from tiki-mytiki_shared.php into a function or a method, and call it when needed.

include_once('tiki-mytiki_shared.php');

10.2.9. Strpos()-like Comparison

10.2.9.1. Piwigo

Strpos()-like Comparison, in admin/include/functions.php:2585.

preg_match may return 0 if not found, and null if the $pattern is erroneous. While hardcoded regex may be checked at compile time, dynamically built regex may fail at execution time. This is particularly important here, since the function may be called with incoming data for maintenance : ‘clear_derivative_cache($_GET[‘type’]);’ is in the /admin/maintenance.php.

function clear_derivative_cache_rec($path, $pattern)
{
  $rmdir = true;
  $rm_index = false;

  if ($contents = opendir($path))
  {
    while (($node = readdir($contents)) !== false)
    {
      if ($node == '.' or $node == '..')
        continue;
      if (is_dir($path.'/'.$node))
      {
        $rmdir &= clear_derivative_cache_rec($path.'/'.$node, $pattern);
      }
      else
      {
        if (preg_match($pattern, $node))

10.2.9.2. Thelia

Strpos()-like Comparison, in core/lib/Thelia/Controller/Admin/FileController.php:198.

preg_match is used here to identify files with a forbidden extension. The actual list of extension is provided to the method via the parameter $extBlackList, which is an array. In case of mis-configuration by the user of this array, preg_match may fail : for example, when regex special characters are provided. At that point, the whole filter becomes invalid, and can’t distinguish good files (returning false) and other files (returning NULL). It is safe to use === false in this situation.

if (!empty($extBlackList)) {
            $regex = "#^(.+)\.(".implode("|", $extBlackList).")$#i";

            if (preg_match($regex, $realFileName)) {
                $message = $this->getTranslator()
                    ->trans(
                        'Files with the following extension are not allowed: %extension, please do an archive of the file if you want to upload it',
                        [
                            '%extension' => $fileBeingUploaded->getClientOriginalExtension(),
                        ]
                    );
            }
        }

10.2.10. var_dump()… Usage

10.2.10.1. Tine20

var_dump()… Usage, in tine20/library/Ajam/Connection.php:122.

Two usage of var_dump(). They are protected by configuration, since the debug property must be set to ‘true’. Yet, it is safer to avoid them altogether, and log the information to an external file.

if($this->debug === true) {
            var_dump($this->getLastRequest());
            var_dump($response);
        }

10.2.10.2. Piwigo

var_dump()… Usage, in include/ws_core.inc.php:273.

This is a hidden debug system : when the response format is not available, the whole object is dumped in the output.

function run()
  {
    if ( is_null($this->_responseEncoder) )
    {
      set_status_header(400);
      @header("Content-Type: text/plain");
      echo ("Cannot process your request. Unknown response format.
Request format: ".@$this->_requestFormat." Response format: ".@$this->_responseFormat."\n");
      var_export($this);
      die(0);
    }

10.2.11. Empty Function

10.2.11.1. Contao

Empty Function, in core-bundle/src/Resources/contao/modules/ModuleQuicklink.php:91.

The closure used with array_map() is empty : this means that the keys are all set to the returned value of the empty closure, which is null. The actual effect is to reset the values to NULL. A better solution, without using the empty closure, is to rely on array_fill_keys() to create an array with default values.

if (!empty($tmp) && \is_array($tmp))
                    {
                            $arrPages = array_map(function () {}, array_flip($tmp));
                    }

10.2.12. Used Once Variables

10.2.12.1. shopware

Used Once Variables, in _sql/migrations/438-add-email-template-header-footer-fields.php:115.

In the updateEmailTemplate method, $generatedQueries collects all the generated SQL queries. $generatedQueries is not initialized, and never used after initialization.

private function updateEmailTemplate($name, $content, $contentHtml = null)
    {
        $sql = <<<SQL
UPDATE `s_core_config_mails` SET `content` = "$content" WHERE `name` = "$name" AND dirty = 0
SQL;
        $this->addSql($sql);

        if ($contentHtml != null) {
            $sql = <<<SQL
UPDATE `s_core_config_mails` SET `content` = "$content", `contentHTML` = "$contentHtml" WHERE `name` = "$name" AND dirty = 0
SQL;
            $generatedQueries[] = $sql;
        }

        $this->addSql($sql);
    }

10.2.12.2. Vanilla

Used Once Variables, in library/core/class.configuration.php:1461.

In this code, $cachedConfigData is collected after storing date in the cache. Gdn::cache()->store() does actual work, so its calling is necessary. The result, collected after execution, is not reused in the rest of the method (long method, not all is shown here). Removing such variable is a needed clean up after development and debug, but also prevents pollution of the variable namespace.

// Save to cache if we're into that sort of thing
                $fileKey = sprintf(Gdn_Configuration::CONFIG_FILE_CACHE_KEY, $this->Source);
                if ($this->Configuration && $this->Configuration->caching() && Gdn::cache()->type() == Gdn_Cache::CACHE_TYPE_MEMORY && Gdn::cache()->activeEnabled()) {
                    $cachedConfigData = Gdn::cache()->store($fileKey, $data, [
                        Gdn_Cache::FEATURE_NOPREFIX => true,
                        Gdn_Cache::FEATURE_EXPIRY => 3600
                    ]);
                }

10.2.13. Empty Classes

10.2.13.1. WordPress

Empty Classes, in wp-includes/SimplePie/Core.php:54.

Empty class, but documented as backward compatibility.

/**
 * SimplePie class.
 *
 * Class for backward compatibility.
 *
 * @deprecated Use {@see SimplePie} directly
 * @package SimplePie
 * @subpackage API
 */
class SimplePie_Core extends SimplePie
{

}

10.2.14. Non Ascii Variables

10.2.14.1. Magento

Non Ascii Variables, in dev/tests/functional/tests/app/Mage/Checkout/Test/Constraint/AssertOrderWithMultishippingSuccessPlacedMessage.php:52.

The initial C is actually a russian C.

$сheckoutMultishippingSuccess

10.2.15. Non Static Methods Called In A Static

10.2.15.1. Dolphin

Non Static Methods Called In A Static, in Dolphin-v.7.3.5/xmlrpc/BxDolXMLRPCFriends.php:11.

getIdByNickname() is indeed defined in the class ‘BxDolXMLRPCUtil’ and it calls the database. The class relies on functions (not methods) to query the database with the correct connexion.

class BxDolXMLRPCFriends
{
    function getFriends($sUser, $sPwd, $sNick, $sLang)
    {
        $iIdProfile = BxDolXMLRPCUtil::getIdByNickname ($sNick);

10.2.15.2. Magento

Non Static Methods Called In A Static, in app/code/core/Mage/Paypal/Model/Payflowlink.php:143.

Mage_Payment_Model_Method_Abstract is an abstract class : this way, it is not possible to instantiate it and then, access its methods. The class is extended, so it could be called from one of the objects. Although, the troubling part is that isAvailable() uses $this, so it can’t be static.

Mage_Payment_Model_Method_Abstract::isAvailable($quote)

10.2.16. Forgotten Visibility

10.2.16.1. FuelCMS

Forgotten Visibility, in /fuel/modules/fuel/controllers/Module.php:713.

Missing visibility for the index() method,and all the methods in the Module class.

class Module extends Fuel_base_controller {

    // --------------------------------------------------------------------

    /**
     * Displays the list (table) view
     *
     * @access      public
     * @return      void
     */
    function index()
    {
            $this->items();
    }

10.2.16.2. LiveZilla

Forgotten Visibility, in livezilla/_lib/objects.global.users.inc.php:2516.

Static method that could be public.

class Visitor extends BaseUser
{
// Lots of code

    static function CreateSPAMFilter($_userId,$_base64=true)
    {
        if(!empty(Server::$Configuration->File[gl_sfa]))
        {

10.2.17. Multiple Index Definition

10.2.17.1. Magento

Multiple Index Definition, in app/code/core/Mage/Adminhtml/Block/System/Convert/Gui/Grid.php:80.

‘type’ is defined twice. The first one, ‘options’ is overwritten.

$this->addColumn('store_id', array(
            'header'    => Mage::helper('adminhtml')->__('Store'),
            'type'      => 'options',
            'align'     => 'center',
            'index'     => 'store_id',
            'type'      => 'store',
            'width'     => '200px',
        ));

10.2.17.2. MediaWiki

Multiple Index Definition, in resources/Resources.php:223.

‘target’ is repeated, though with the same values. This is just dead code.

// inside a big array
    'jquery.getAttrs' => [
            'targets' => [ 'desktop', 'mobile' ],
            'scripts' => 'resources/src/jquery/jquery.getAttrs.js',
            'targets' => [ 'desktop', 'mobile' ],
    ],
    // big array continues

10.2.18. Incompilable Files

10.2.18.1. xataface

Incompilable Files, in lib/XML/Tree.php:289.

Compilation error with PHP 7.2 version.

syntax error, unexpected 'new' (T_NEW)

10.2.19. Multiple Constant Definition

10.2.19.1. Dolibarr

Multiple Constant Definition, in htdocs/main.inc.php:914.

All is documented here : ‘Constants used to defined number of lines in textarea’. Constants are not changing during an execution, and this allows the script to set values early in the process, and have them used later, in the templates. Yet, building constants dynamically may lead to confusion, when developpers are not aware of the change.

// Constants used to defined number of lines in textarea
if (empty($conf->browser->firefox))
{
    define('ROWS_1',1);
    define('ROWS_2',2);
    define('ROWS_3',3);
    define('ROWS_4',4);
    define('ROWS_5',5);
    define('ROWS_6',6);
    define('ROWS_7',7);
    define('ROWS_8',8);
    define('ROWS_9',9);
}
else
{
    define('ROWS_1',0);
    define('ROWS_2',1);
    define('ROWS_3',2);
    define('ROWS_4',3);
    define('ROWS_5',4);
    define('ROWS_6',5);
    define('ROWS_7',6);
    define('ROWS_8',7);
    define('ROWS_9',8);
}

10.2.19.2. OpenConf

Multiple Constant Definition, in modules/request.php:71.

The constant is build according to the situation, in the part of the script (file request.php). This hides the actual origin of the value, but keeps the rest of the code simple. Just keep in mind that this constant may have different values.

if (isset($_GET['ocparams']) && !empty($_GET['ocparams'])) {
            $params = '';
            if (preg_match_all("/(\w+)--(\w+)_-/", $_GET['ocparams'], $matches)) {
                    foreach ($matches[1] as $idx => $m) {
                            if (($m != 'module') && ($m != 'action') && preg_match("/^[\w-]+$/", $m)) {
                                    $params .= '&' . $m . '=' . urlencode($matches[2][$idx]);
                                    $_GET[$m] = $matches[2][$idx];
                            }
                    }
            }
            unset($_GET['ocparams']);
            define('OCC_SELF', $_SERVER['PHP_SELF'] . '?module=' . $_REQUEST['module'] . '&action=' . $_GET['action'] . $params);
    } elseif (isset($_SERVER['REQUEST_URI']) && strstr($_SERVER['REQUEST_URI'], '?')) {
            define('OCC_SELF', htmlspecialchars($_SERVER['REQUEST_URI']));
    } elseif (isset($_SERVER['QUERY_STRING']) && strstr($_SERVER['QUERY_STRING'], '&')) {
            define('OCC_SELF', $_SERVER['PHP_SELF'] . '?' . htmlspecialchars($_SERVER['QUERY_STRING']));
    } else {
            err('This server does not support REQUEST_URI or QUERY_STRING','Error');
    }

10.2.20. Invalid Constant Name

10.2.20.1. OpenEMR

Invalid Constant Name, in library/classes/InsuranceCompany.class.php:20.

Either a copy/paste, or a generated definition file : the file contains 25 constants definition. The constant is not found in the rest of the code.

define("INS_TYPE_OTHER_NON-FEDERAL_PROGRAMS", 10);

10.2.21. Wrong Optional Parameter

10.2.21.1. FuelCMS

Wrong Optional Parameter, in fuel/modules/fuel/helpers/validator_helper.php:78.

The $regex parameter should really be first, as it is compulsory. Though, if this is a legacy function, it may be better to give regex a default value, such as empty string or null, and test it before using it.

if (!function_exists('regex'))
{
    function regex($var = null, $regex)
    {
            return preg_match('#'.$regex.'#', $var);
    }
}

10.2.21.2. Vanilla

Wrong Optional Parameter, in fuel/modules/fuel/helpers/validator_helper.php:78.

Note the second parameter, $dropdown, which has no default value. It is relayed to the addDropdown method, which as no default value too. Since both methods are documented, we can see that they should be an addDropdown : null is probably a good idea, coupled with an explicit check on the actual value.

/**
     * Add a dropdown to the items array if it satisfies the $isAllowed condition.
     *
     * @param bool|string|array $isAllowed Either a boolean to indicate whether to actually add the item
     * or a permission string or array of permission strings (full match) to check.
     * @param DropdownModule $dropdown The dropdown menu to add.
     * @param string $key The item's key (for sorting and CSS targeting).
     * @param string $cssClass The dropdown wrapper's CSS class.
     * @param array|int $sort Either a numeric sort position or and array in the style: array('before|after', 'key').
     * @return NavModule $this The calling object.
     */
    public function addDropdownIf($isAllowed = true, $dropdown, $key = '', $cssClass = '', $sort = []) {
        if (!$this->isAllowed($isAllowed)) {
            return $this;
        } else {
            return $this->addDropdown($dropdown, $key, $cssClass, $sort);
        }
    }

10.2.22. One Variable String

10.2.22.1. Tikiwiki

One Variable String, in lib/wiki-plugins/wikiplugin_addtocart.php:228.

Double-quotes are not needed here. If casting to string is important, the (string) would be more explicit.

foreach ($plugininfo['params'] as $key => $param) {
            $default["$key"] = $param['default'];
    }

10.2.22.2. NextCloud

One Variable String, in build/integration/features/bootstrap/BasicStructure.php:349.

Both concatenations could be merged, independantly. If readability is important, why not put them inside curly brackets?

public static function removeFile($path, $filename) {
            if (file_exists("$path" . "$filename")) {
                    unlink("$path" . "$filename");
            }
    }

10.2.23. Static Methods Can’t Contain $this

10.2.23.1. xataface

Static Methods Can’t Contain $this, in Dataface/LanguageTool.php:48.

$this is hidden in the arguments of the static call to the method.

public static function loadRealm($name){
            return self::getInstance($this->app->_conf['default_language'])->loadRealm($name);
    }

10.2.23.2. SugarCrm

Static Methods Can’t Contain $this, in SugarCE-Full-6.5.26/modules/ACLActions/ACLAction.php:332.

Notice how $this is tested for existence before using it. It seems strange, at first, but we have to remember that if $this is never set when calling a static method, a static method may be called with $this. Confusingly, this static method may be called in two ways.

static function hasAccess($is_owner=false, $access = 0){

        if($access != 0 && $access == ACL_ALLOW_ALL || ($is_owner && $access == ACL_ALLOW_OWNER))return true;
       //if this exists, then this function is not static, so check the aclaccess parameter
        if(isset($this) && isset($this->aclaccess)){
            if($this->aclaccess == ACL_ALLOW_ALL || ($is_owner && $this->aclaccess == ACL_ALLOW_OWNER))
            return true;
        }
        return false;
    }

10.2.24. While(List() = Each())

10.2.24.1. OpenEMR

While(List() = Each()), in library/report.inc:153.

The first while() is needed, to read the arbitrary long list returned by the SQL query. The second list may be upgraded with a foreach, to read both the key and the value. This is certainly faster to execute and to read.

function getInsuranceReport($pid, $type = primary)
{
    $sql = select * from insurance_data where pid=? and type=? order by date ASC;
    $res = sqlStatement($sql, array($pid, $type));
    while ($list = sqlFetchArray($res)) {
        while (list($key, $value) = each($list)) {
            if ($ret[$key]['content'] != $value && $ret[$key]['date'] < $list['date']) {
                $ret[$key]['content'] = $value;
                $ret[$key]['date'] = $list['date'];
            }
        }
    }

    return $ret;
}

10.2.24.2. Dolphin

While(List() = Each()), in Dolphin-v.7.3.5/modules/boonex/forum/classes/Forum.php:1875.

This clever use of while() and list() is actually a foreach($a as $r) (the keys are ignored)

function getRssUpdatedTopics ()
    {
        global $gConf;

        $this->_rssPrepareConf ();

        $a = $this->fdb->getRecentTopics (0);

        $items = '';
        $lastBuildDate = '';
        $ui = array();
        reset ($a);
        while ( list (,$r) = each ($a) ) {
            // acquire user info
            if (!isset($ui[$r['last_post_user']]) && ($aa = $this->_getUserInfoReadyArray ($r['last_post_user'], false)))
                $ui[$r['last_post_user']] = $aa;

            $td = orca_mb_replace('/#/', $r['count_posts'], '[L[# posts]]') . ' &#183; ' . orca_mb_replace('/#/', $ui[$r['last_post_user']]['title'], '[L[last reply by #]]') . ' &#183; ' . $r['cat_name'] . ' &#187; ' . $r['forum_title'];

10.2.25. Several Instructions On The Same Line

10.2.25.1. Piwigo

Several Instructions On The Same Line, in tools/triggers_list.php:993.

There are two instructions on the line with the if(). Note that the condition is not followed by a bracketed block. When reviewing, it really seems that echo ‘<br>’ and $f=0; are on the same block, but the second is indeed an unconditional expression. This is very difficult to spot.

foreach ($trigger['files'] as $file)
      {
        if (!$f) echo '<br>'; $f=0;
        echo preg_replace('#\((.+)\)#', '(<i>$1</i>)', $file);
      }

10.2.25.2. Tine20

Several Instructions On The Same Line, in tine20/Calendar/Controller/Event.php:1594.

Here, $_event->attendee is saved in a local variable, then the property is destroyed. Same for $_event->notes; Strangely, a few lines above, the properties are unset on their own line. Unsetting properties leads to surprise bugs, and hidding the unset after ; makes it harder to spot.

$futurePersistentExceptionEvents->setRecurId($_event->getId());
                unset($_event->recurid);
                unset($_event->base_event_id);
                foreach(array('attendee', 'notes', 'alarms') as $prop) {
                    if ($_event->{$prop} instanceof Tinebase_Record_RecordSet) {
                        $_event->{$prop}->setId(NULL);
                    }
                }
                $_event->exdate = $futureExdates;

                $attendees = $_event->attendee; unset($_event->attendee);
                $note = $_event->notes; unset($_event->notes);
                $persistentExceptionEvent = $this->create($_event, $_checkBusyConflicts && $dtStartHasDiff);

10.2.26. Multiples Identical Case

10.2.26.1. SugarCrm

Multiples Identical Case, in modules/ModuleBuilder/MB/MBPackage.php:439.

It takes a while to find the double ‘required’ case, but the executed code is actually the same, so this is dead code at worst.

switch ($col) {
    case 'custom_module':
            $installdefs['custom_fields'][$name]['module'] = $res;
            break;
    case 'required':
            $installdefs['custom_fields'][$name]['require_option'] = $res;
            break;
    case 'vname':
            $installdefs['custom_fields'][$name]['label'] = $res;
            break;
    case 'required':
            $installdefs['custom_fields'][$name]['require_option'] = $res;
            break;
    case 'massupdate':
            $installdefs['custom_fields'][$name]['mass_update'] = $res;
            break;
    case 'comments':
            $installdefs['custom_fields'][$name]['comments'] = $res;
            break;
    case 'help':
            $installdefs['custom_fields'][$name]['help'] = $res;
            break;
    case 'len':
            $installdefs['custom_fields'][$name]['max_size'] = $res;
            break;
    default:
            $installdefs['custom_fields'][$name][$col] = $res;
}//switch

10.2.26.2. ExpressionEngine

Multiples Identical Case, in ExpressionEngine_Core2.9.2/system/expressionengine/controllers/cp/admin_content.php:577.

‘deft_status’ is doubled, with a fallthrough. This looks like some forgotten copy/paste.

switch ($key){
                                                            case 'cat_group':
                                                                //PHP code
                                                                    break;
                                                            case 'status_group':
                                                            case 'field_group':
                                                                //PHP code
                                                                    break;
                                                            case 'deft_status':
                                                            case 'deft_status':
                                                                //PHP code
                                                                    break;
                                                            case 'search_excerpt':
                                                                //PHP code
                                                                    break;
                                                            case 'deft_category':
                                                                //PHP code
                                                                    break;
                                                            case 'blog_url':
                                                            case 'comment_url':
                                                            case 'search_results_url':
                                                            case 'rss_url':
                                                                //PHP code
                                                                    break;
                                                            default :
                                                                //PHP code
                                                                    break;
                                                    }

10.2.27. Switch Without Default

10.2.27.1. Zencart

Switch Without Default, in admin/tax_rates.php:15.

The ‘action’ is collected from $_GET and then, compared with various strings to handle the different actions to be taken. The default behavior is implicit here : if no ‘action’, display the initial form for taxes to be changed. This has to be understood as a general philosophy of ZenCart project, or by reading the rest of the HTML code. Adding a ‘default’ case here would help understand what happens in case ‘action’ is absent or unrecognized.

$action = (isset($_GET['action']) ? $_GET['action'] : '');

  if (zen_not_null($action)) {
    switch ($action) {
      case 'insert':
        // PHP code
        break;
      case 'save':
        // PHP code
        break;
      case 'deleteconfirm':
        // PHP code
        break;
    }
  }
?> .... HTML code

10.2.27.2. Traq

Switch Without Default, in src/Helpers/Ticketlist.php:311.

The default case is actually processed after the switch, by the next if/then structure. The structure deals with the customFields, while the else deals with any unknown situations. This if/then could be wrapped in the ‘default’ case of switch, for consistent processing. The if/then condition would be hard to use as a ‘case’ (possible, though).

public static function dataFor($column, $ticket)
    {
        switch ($column) {
            // Ticket ID column
            case 'ticket_id':
                return $ticket['ticket_id'];
                break;

            // Status column
            case 'status':
            case 'type':
            case 'component':
            case 'priority':
            case 'severity':
                return $ticket[{$column}_name];
                break;

            // Votes
            case 'votes':
                return $ticket['votes'];
                break;
        }

        // If we're still here, it may be a custom field
        if ($value = $ticket->customFieldValue($column)) {
            return $value->value;
        }

        // Nothing!
        return '';
    }

10.2.28. $this Belongs To Classes Or Traits

10.2.28.1. OpenEMR

$this Belongs To Classes Or Traits, in ccr/display.php:24.

$this is used to call the document_upload_download_log() method, although this piece of code is not part of a class, nor is included in a class.

<?php
require_once(dirname(__FILE__) . "/../interface/globals.php");

$type = $_GET['type'];
$document_id = $_GET['doc_id'];
$d = new Document($document_id);
$url =  $d->get_url();
$storagemethod = $d->get_storagemethod();
$couch_docid = $d->get_couch_docid();
$couch_revid = $d->get_couch_revid();

if ($couch_docid && $couch_revid) {
    $couch = new CouchDB();
    $data = array($GLOBALS['couchdb_dbase'],$couch_docid);
    $resp = $couch->retrieve_doc($data);
    $xml = base64_decode($resp->data);
    if ($content=='' && $GLOBALS['couchdb_log']==1) {
        $log_content = date('Y-m-d H:i:s')." ==> Retrieving document\r\n";
        $log_content = date('Y-m-d H:i:s')." ==> URL: ".$url."\r\n";
        $log_content .= date('Y-m-d H:i:s')." ==> CouchDB Document Id: ".$couch_docid."\r\n";
        $log_content .= date('Y-m-d H:i:s')." ==> CouchDB Revision Id: ".$couch_revid."\r\n";
        $log_content .= date('Y-m-d H:i:s')." ==> Failed to fetch document content from CouchDB.\r\n";
        //$log_content .= date('Y-m-d H:i:s')." ==> Will try to download file from HardDisk if exists.\r\n\r\n";
        $this->document_upload_download_log($d->get_foreign_id(), $log_content);
        die(xlt("File retrieval from CouchDB failed"));
    }

10.2.29. Nested Ternary

10.2.29.1. SPIP

Nested Ternary, in ecrire/inc/utils.php:2648.

Interesting usage of both if/then, for the flow control, and ternary, for data process. Even on multiple lines, nested ternaries are quite hard to read.

// le script de l'espace prive
    // Mettre a "index.php" si DirectoryIndex ne le fait pas ou pb connexes:
    // les anciens IIS n'acceptent pas les POST sur ecrire/ (#419)
    // meme pb sur thttpd cf. http://forum.spip.net/fr_184153.html
    if (!defined('_SPIP_ECRIRE_SCRIPT')) {
            define('_SPIP_ECRIRE_SCRIPT', (empty($_SERVER['SERVER_SOFTWARE']) ? '' :
                    preg_match(',IIS|thttpd,', $_SERVER['SERVER_SOFTWARE']) ?
                            'index.php' : ''));
    }

10.2.29.2. Zencart

Nested Ternary, in ecrire/inc/utils.php:2648.

No more than one level of nesting for this ternary call, yet it feels a lot more, thanks to the usage of arrayed properties, constants, and functioncalls.

$lc_text .= '<br />' . (zen_get_show_product_switch($listing->fields['products_id'], 'ALWAYS_FREE_SHIPPING_IMAGE_SWITCH') ? (zen_get_product_is_always_free_shipping($listing->fields['products_id']) ? TEXT_PRODUCT_FREE_SHIPPING_ICON . '<br />' : '') : '');

10.2.30. Non-constant Index In Array

10.2.30.1. Dolibarr

Non-constant Index In Array, in htdocs/includes/OAuth/Common/Storage/DoliStorage.php:245.

The state constant in the $result array is coming from the SQL query. There is no need to make this a constant : making it a string will remove some warnings in the logs.

public function hasAuthorizationState($service)
    {
        // get state from db
        dol_syslog("get state from db");
        $sql = "SELECT state FROM ".MAIN_DB_PREFIX."oauth_state";
        $sql.= " WHERE service='".$this->db->escape($service)."'";
        $resql = $this->db->query($sql);
        $result = $this->db->fetch_array($resql);
        $states[$service] = $result[state];
        $this->states[$service] = $states[$service];

        return is_array($states)
        && isset($states[$service])
        && null !== $states[$service];
    }

10.2.30.2. Zencart

Non-constant Index In Array, in app/library/zencart/Services/src/LeadLanguagesRoutes.php:112.

The fields constant in the $tableEntry which holds a list of tables. It seems to be a SQL result, but it is conveniently abstracted with $this->listener->getTableList(), so we can’t be sure.

public function updateLanguageTables($insertId)
    {
        $tableList = $this->listener->getTableList();
        if (count($tableList) == 0) {
            return;
        }
        foreach ($tableList as $tableEntry) {
            $languageKeyField = issetorArray($tableEntry, 'languageKeyField', 'language_id');
            $sql = " INSERT IGNORE INTO :table: (";
            $sql = $this->dbConn->bindVars($sql, ':table:', $tableEntry ['table'], 'noquotestring');
            $sql .= $languageKeyField. ", ";
            $fieldNames = "";
            foreach ($tableEntry[fields] as $fieldName => $fieldType) {
                $fieldNames .= $fieldName . ", ";
            }

10.2.31. Class, Interface Or Trait With Identical Names

10.2.31.1. shopware

Class, Interface Or Trait With Identical Names, in engine/Shopware/Components/Form/Interfaces/Element.php:30.

Most Element classes extends ModelEntity, which is an abstract class. There is also an interface, called Element, for forms. And, last, one of the class Element extends JsonSerializable, which is a PHP native interface. Namespaces are definitely crucial to understand which Element is which.

interface Element { /**/ } // in engine/Shopware/Components/Form/Interfaces/Element.php:30

class Element implements \JsonSerializable { /**/ }         // in engine/Shopware/Bundle/EmotionBundle/Struct/Element.php:29

class Element extends ModelEntity { /**/ }  // in /engine/Shopware/Models/Document/Element.php:37

10.2.31.2. NextCloud

Class, Interface Or Trait With Identical Names, in lib/private/Files/Storage/Storage.php:33.

Interface Storage extends another Storage class. Here, the fully qualified name is used, so we can understand which storage is which at read time : a ‘use’ alias would make this line more confusing.

interface Storage extends \OCP\Files\Storage { /**/ }

10.2.32. Empty Try Catch

10.2.32.1. LiveZilla

Empty Try Catch, in livezilla/_lib/trdp/Zend/Mail/Protocol/Pop3.php:237.

This is an aptly commented empty try/catch : the emited exception is extra check for a Zend Mail Protocol Exception. Hopefully, the Zend_Mail_Protocol_Exception only covers a already-closed situation. Anyhow, this should be logged for later diagnostic.

public function logout()
    {
        if (!$this->_socket) {
            return;
        }

        try {
            $this->request('QUIT');
        } catch (Zend_Mail_Protocol_Exception $e) {
            // ignore error - we're closing the socket anyway
        }

        fclose($this->_socket);
        $this->_socket = null;
    }

10.2.32.2. Mautic

Empty Try Catch, in livezilla/_lib/trdp/Zend/Mail/Protocol/Pop3.php:237.

Removing a file : if the file is not ‘deleted’ by the method call, but raises an error, it is hidden. When file destruction is impossible because the file is already destroyed (or missing), this is well. If the file couldn’t be destroyed because of missing writing privileges, hiding this error will have serious consequences.

/**
     * @param string $fileName
     */
    public function removeFile($fileName)
    {
        try {
            $path = $this->getPath($fileName);
            $this->filePathResolver->delete($path);
        } catch (FileIOException $e) {
        }
    }

10.2.33. Used Once Variables (In Scope)

10.2.33.1. shopware

Used Once Variables (In Scope), in _sql/migrations/438-add-email-template-header-footer-fields.php:115.

In the updateEmailTemplate method, $generatedQueries collects all the generated SQL queries. $generatedQueries is not initialized, and never used after initialization.

private function updateEmailTemplate($name, $content, $contentHtml = null)
    {
        $sql = <<<SQL
UPDATE `s_core_config_mails` SET `content` = "$content" WHERE `name` = "$name" AND dirty = 0
SQL;
        $this->addSql($sql);

        if ($contentHtml != null) {
            $sql = <<<SQL
UPDATE `s_core_config_mails` SET `content` = "$content", `contentHTML` = "$contentHtml" WHERE `name` = "$name" AND dirty = 0
SQL;
            $generatedQueries[] = $sql;
        }

        $this->addSql($sql);
    }

10.2.34. Deprecated Functions

10.2.34.1. Dolphin

Deprecated Functions, in Dolphin-v.7.3.5/inc/classes/BxDolAdminSettings.php:270.

Split() was abandonned in PHP 7.0

split(',', $aItem['extra']);

10.2.35. Dangling Array References

10.2.35.1. Typo3

Dangling Array References, in typo3/sysext/impexp/Classes/ImportExport.php:322.

foreach() reads $lines into $r, and augment those lines. By the end, the $r variable is not unset. Yet, several lines later, in the same method but with different conditions, another loop reuse the variable $r. If is_array($this->dat[‘header’][‘pagetree’] and is_array($this->remainHeader[‘records’]) are arrays at the same moment, then both loops are called, and they share the same reference. Values of the latter array will end up in the formar.

if (is_array($this->dat['header']['pagetree'])) {
            reset($this->dat['header']['pagetree']);
            $lines = [];
            $this->traversePageTree($this->dat['header']['pagetree'], $lines);

            $viewData['dat'] = $this->dat;
            $viewData['update'] = $this->update;
            $viewData['showDiff'] = $this->showDiff;
            if (!empty($lines)) {
                foreach ($lines as &$r) {
                    $r['controls'] = $this->renderControls($r);
                    $r['fileSize'] = GeneralUtility::formatSize($r['size']);
                    $r['message'] = ($r['msg'] && !$this->doesImport ? '<span class=text-danger>' . htmlspecialchars($r['msg']) . '</span>' : '');
                }
                $viewData['pagetreeLines'] = $lines;
            } else {
                $viewData['pagetreeLines'] = [];
            }
        }
        // Print remaining records that were not contained inside the page tree:
        if (is_array($this->remainHeader['records'])) {
            $lines = [];
            if (is_array($this->remainHeader['records']['pages'])) {
                $this->traversePageRecords($this->remainHeader['records']['pages'], $lines);
            }
            $this->traverseAllRecords($this->remainHeader['records'], $lines);
            if (!empty($lines)) {
                foreach ($lines as &$r) {
                    $r['controls'] = $this->renderControls($r);
                    $r['fileSize'] = GeneralUtility::formatSize($r['size']);
                    $r['message'] = ($r['msg'] && !$this->doesImport ? '<span class=text-danger>' . htmlspecialchars($r['msg']) . '</span>' : '');
                }
                $viewData['remainingRecords'] = $lines;
            }
        }

10.2.35.2. SugarCrm

Dangling Array References, in typo3/sysext/impexp/Classes/ImportExport.php:322.

There are two nested foreach here : they both have referenced blind variables. The second one uses $data, but never changes it. Yet, it is reused the next round in the first loop, leading to pollution from the first rows of $this->_parser->data into the lasts. This may happen even if $data is not modified explicitely : in fact, it will be modified the next call to foreach($row as …), for each element in $row.

foreach ($this->_parser->data as &$row) {
                foreach ($row as &$data) {
                    $len = strlen($data);
                    // check if it begins and ends with single quotes
                    // if it does, then it double quotes may not be the enclosure
                    if ($len>=2 && $data[0] == " && $data[$len-1] == ") {
                        $beginEndWithSingle = true;
                        break;
                    }
                }
                if ($beginEndWithSingle) {
                    break;
                }
                $depth++;
                if ($depth > $this->_max_depth) {
                    break;
                }
            }

10.2.36. Aliases Usage

10.2.36.1. Cleverstyle

Aliases Usage, in modules/HybridAuth/Hybrid/thirdparty/Vimeo/Vimeo.php:422.

is_writeable() should be written is_writable(). No extra ‘e’.

is_writeable($chunk_temp_dir)

10.2.36.2. phpMyAdmin

Aliases Usage, in libraries/classes/Server/Privileges.php:5064.

join() should be written implode()

join('`, `', $tmp_privs2['Update'])

10.2.37. Var Keyword

10.2.37.1. xataface

Var Keyword, in SQL/Parser/wrapper.php:24.

With the usage of var and a first method bearing the name of the class, this is PHP 4 code that is still in use.

class SQL_Parser_wrapper {

    var $_data;
    var $_tableLookup;
    var $_parser;

    function SQL_Parser_wrapper(&$data, $dialect='MySQL'){

10.2.38. Wrong Number Of Arguments

10.2.38.1. xataface

Wrong Number Of Arguments, in actions/existing_related_record.php:130.

df_display() actually requires only 2 arguments, while three are provided. The last argument is completely ignored. df_display() is called in a total of 9 places : this now looks like an API change that left many calls untouched.

df_display($context, $template, true);

// in public-api.php :
function df_display($context, $template_name){
    import( 'Dataface/SkinTool.php');
    $st = Dataface_SkinTool::getInstance();

    return $st->display($context, $template_name);
}

10.2.39. Undefined static:: Or self::

10.2.39.1. xataface

Undefined static:: Or self::, in actions/forgot_password.php:194.

This is probably a typo, since the property called public static $EX_NO_USERS_WITH_EMAIL = 501; is defined in that class.

if ( !$user ) throw new Exception(df_translate('actions.forgot_password.null_user',"Cannot send email for null user"), self::$EX_NO_USERS_FOUND_WITH_EMAIL);

10.2.39.2. SugarCrm

Undefined static:: Or self::, in actions/forgot_password.php:194.

self::$sugar_strptime_long_mon refers to the current class, which extends DateTime. No static property was defined at either of them, with the name ‘$sugar_strptime_long_mon’. This has been a Fatal error at execution time since PHP 5.3, at least.

if ( isset($regexp['positions']['F']) && !empty($dateparts[$regexp['positions']['F']])) {
                       // FIXME: locale?
            $mon = $dateparts[$regexp['positions']['F']];
            if(isset(self::$sugar_strptime_long_mon[$mon])) {
                $data["tm_mon"] = self::$sugar_strptime_long_mon[$mon];
            } else {
                return false;
            }
        }

10.2.40. list() May Omit Variables

10.2.40.1. OpenConf

list() May Omit Variables, in openconf/author/privacy.php:29.

The first variable in the list(), $none, isn’t reused anywhere in the script. In fact, its name convey the meaning that is it useless, but is in the array nonetheless.

list($none, $OC_privacy_policy) = oc_getTemplate('privacy_policy');

10.2.40.2. FuelCMS

list() May Omit Variables, in wp-admin/includes/misc.php:74.

$a is never reused again. $b, on the other hand is. Not assigning any value to $a saves some memory, and avoid polluting the local variable space.

list($b, $a) = array(reset($params->me), key($params->me));

10.2.41. Or Die

10.2.41.1. Tine20

Or Die, in scripts/addgrant.php:34.

Typical error handling, which also displays the MySQL error message, and leaks informations about the system. One may also note that mysql_connect is not supported anymore, and was replaced with mysqli and pdo : this may be a backward compatibile file.

$link = mysql_connect($host, $user, $pass) or die("No connection: " . mysql_error( ))

10.2.41.2. OpenConf

Or Die, in openconf/chair/export.inc:143.

or die() is also applied to many situations, where a blocking situation arise. Here, with the creation of a temporary file.

$coreFile = tempnam('/tmp/', 'ocexport') or die('could not generate Excel file (6)')

10.2.42. Use const

10.2.42.1. phpMyAdmin

Use const, in error_report.php:17.

This may be turned into a const call, with a static expression.

define('ROOT_PATH', __DIR__ . DIRECTORY_SEPARATOR)

10.2.42.2. Piwigo

Use const, in include/functions_plugins.inc.php:32.

Const works efficiently with literal

define('EVENT_HANDLER_PRIORITY_NEUTRAL', 50)

10.2.43. Foreach Reference Is Not Modified

10.2.43.1. Dolibarr

Foreach Reference Is Not Modified, in htdocs/product/reassort.php:364.

$data is read for its

foreach ($this->_parser->data as &$row) {
                foreach ($row as &$data) {
                    $len = strlen($data);
                    // check if it begins and ends with single quotes
                    // if it does, then it double quotes may not be the enclosure
                    if ($len>=2 && $data[0] == "'" && $data[$len-1] == "'") {
                        $beginEndWithSingle = true;
                        break;
                    }
                }
                if ($beginEndWithSingle) {
                    break;
                }
                $depth++;
                if ($depth > $this->_max_depth) {
                    break;
                }
            }

10.2.44. Useless Return

10.2.44.1. ThinkPHP

Useless Return, in library/think/Request.php:2121.

__set() doesn’t need a return, unlike __get().

public function __set($name, $value)
    {
        return $this->param[$name] = $value;
    }

10.2.44.2. Vanilla

Useless Return, in applications/dashboard/views/attachments/attachment.php:14.

The final ‘return’ is useless : return void (here, return without argument), is the same as returning null, unless the ‘void’ return type is used. The other return, is in the two conditions, is important to skip the end of the functioncall.

function writeAttachment($attachment) {

        $customMethod = AttachmentModel::getWriteAttachmentMethodName($attachment['Type']);
        if (function_exists($customMethod)) {
            if (val('Error', $attachment)) {
                writeErrorAttachment($attachment);
                return;
            }
            $customMethod($attachment);
        } else {
            trace($customMethod, 'Write Attachment method not found');
            trace($attachment, 'Attachment');
        }
        return;
    }

10.2.45. Unpreprocessed Values

10.2.45.1. Zurmo

Unpreprocessed Values, in app/protected/core/utils/ZurmoTranslationServerUtil.php:79.

It seems that a simple concatenation could be used here. There is another call to this expression in the code, and a third that uses ‘PATCH_VERSION’ on top of the two others.

join('.', array(MAJOR_VERSION, MINOR_VERSION))

10.2.45.2. Piwigo

Unpreprocessed Values, in include/random_compat/random.php:34.

PHP_VERSION is actually build with PHP_MAJOR_VERSION, PHP_MINOR_VERSION and PHP_RELEASE_VERSION. There is also a compact version : PHP_VERSION_ID

explode('.', PHP_VERSION);

10.2.46. Undefined Properties

10.2.46.1. WordPress

Undefined Properties, in wp-admin/includes/misc.php:74.

Properties are not defined, but they are thoroughly initialized when the XML document is parsed. All those definition should be in a property definition, for clear documentation.

$this->DeliveryLine1 = '';
        $this->DeliveryLine2 = '';
        $this->City = '';
        $this->State = '';
        $this->ZipAddon = '';

10.2.46.2. MediaWiki

Undefined Properties, in wp-admin/includes/misc.php:74.

parsedParametersDeleteLog is an undefined property. Defining the property with a null default value is important here, to keep the code running.

protected function getMessageParameters() {
            if ( isset( $this->parsedParametersDeleteLog ) ) {
                    return $this->parsedParametersDeleteLog;
            }

10.2.47. Strict Comparison With Booleans

10.2.47.1. Phinx

Strict Comparison With Booleans, in src/Phinx/Db/Adapter/MysqlAdapter.php:1131.

ìsNull( )` always returns a boolean : it may be only be true or false. Until typehinted properties or return typehint are used, isNull() may return anything else.

$column->isNull( ) == false

10.2.47.2. Typo3

Strict Comparison With Booleans, in typo3/sysext/lowlevel/Classes/Command/FilesWithMultipleReferencesCommand.php:90.

When dry-run is not defined, the getOption() method actually returns a null value. So, comparing the result of getOption() to false is actually wrong : using a constant to prevent values to be inconsistent is recommended here.

$input->getOption('dry-run') != false

10.2.48. Lone Blocks

10.2.48.1. ThinkPHP

Lone Blocks, in ThinkPHP/Library/Vendor/Hprose/HproseReader.php:163.

There is no need for block in a case/default clause. PHP executes all command in order, until a break or the end of the switch. There is another occurrence of that situation in this code : it seems to be a coding convention, while only applied to a few switch statements.

for ($i = 0; $i < $len; ++$i) {
            switch (ord($this->stream->getc()) >> 4) {
                case 0:
                case 1:
                case 2:
                case 3:
                case 4:
                case 5:
                case 6:
                case 7: {
                    // 0xxx xxxx
                    $utf8len++;
                    break;
                }
                case 12:
                case 13: {
                    // 110x xxxx   10xx xxxx
                    $this->stream->skip(1);
                    $utf8len += 2;
                    break;
                }

10.2.48.2. Tine20

Lone Blocks, in tine20/Addressbook/Convert/Contact/VCard/Abstract.php:199.

A case of empty case, with empty blocks. This is useless code. Event the curly brackets with the final case are useless.

switch ( $property['TYPE'] ) {
                        case 'JPG' : {}
                        case 'jpg' : {}
                        case 'Jpg' : {}
                        case 'Jpeg' : {}
                        case 'jpeg' : {}
                        case 'PNG' : {}
                        case 'png' : {}
                        case 'JPEG' : {
                            if (Tinebase_Core::isLogLevel(Zend_Log::DEBUG))
                                Tinebase_Core::getLogger()->warn(__METHOD__ . '::' . __LINE__ . ' Photo: passing on invalid ' . $property['TYPE'] . ' image as is (' . strlen($property->getValue()) .')' );
                            $jpegphoto = $property->getValue();
                            break;
                        }

10.2.49. PHP Keywords As Names

10.2.49.1. ChurchCRM

PHP Keywords As Names, in src/kiosk/index.php:42.

$false may be true or false (or else…). In fact, the variable is not even defined in this file, and the file do a lot of inclusion.

if (!isset($_COOKIE['kioskCookie'])) {
    if ($windowOpen) {
        $guid = uniqid();
        setcookie("kioskCookie", $guid, 2147483647);
        $Kiosk = new \ChurchCRM\KioskDevice();
        $Kiosk->setGUIDHash(hash('sha256', $guid));
        $Kiosk->setAccepted($false);
        $Kiosk->save();
    } else {
        header("HTTP/1.1 401 Unauthorized");
        exit;
    }
}

10.2.49.2. xataface

PHP Keywords As Names, in Dataface/Record.php:1278.

This one is documented, and in the end, makes a lot of sense.

function &getRelatedRecord($relationshipName, $index=0, $where=0, $sort=0){
            if ( isset($this->cache[__FUNCTION__][$relationshipName][$index][$where][$sort]) ){
                    return $this->cache[__FUNCTION__][$relationshipName][$index][$where][$sort];
            }
            $it = $this->getRelationshipIterator($relationshipName, $index, 1, $where, $sort);
            if ( $it->hasNext() ){
                    $rec =& $it->next();
                    $this->cache[__FUNCTION__][$relationshipName][$index][$where][$sort] =& $rec;
                    return $rec;
            } else {
                    $null = null;   // stupid hack because literal 'null' can't be returned by ref.
                    return $null;
            }
    }

10.2.50. Could Use self

10.2.50.1. WordPress

Could Use self, in wp-admin/includes/misc.php:74.

Securimage could be called self.

class Securimage
{
// Lots of code
            Securimage::$_captchaId = $id;
}

10.2.50.2. LiveZilla

Could Use self, in wp-admin/includes/misc.php:74.

Using self makes it obvious that Operator::GetSystemId() is a local call, while Communication::GetParameter() is external.

class Operator extends BaseUser
{
            $userid = Communication::GetParameter(operator,,$c,FILTER_SANITIZE_SPECIAL_CHARS,null,32,false,false);
            $sysid = Operator::GetSystemId($userid);
}

10.2.51. Logical Should Use Symbolic Operators

10.2.51.1. Cleverstyle

Logical Should Use Symbolic Operators, in modules/Uploader/Mime/Mime.php:171.

$extension is assigned with the results of pathinfo($reference_name, PATHINFO_EXTENSION) and ignores static::hasExtension($extension). The same expression, placed in a condition (like an if), would assign a value to $extension and use another for the condition itself. Here, this code is only an expression in the flow.

$extension = pathinfo($reference_name, PATHINFO_EXTENSION) and static::hasExtension($extension);

10.2.51.2. OpenConf

Logical Should Use Symbolic Operators, in chair/export.inc:143.

In this context, the priority of execution is used on purpose; $coreFile only collect the temporary name of the export file, and when this name is empty, then the second operand of OR is executed, though never collected. Since this second argument is a ‘die’, its return value is lost, but the initial assignation is never used anyway.

$coreFile = tempnam('/tmp/', 'ocexport') or die('could not generate Excel file (6)')

10.2.52. Catch Overwrite Variable

10.2.52.1. PhpIPAM

Catch Overwrite Variable, in app/subnets/scan/subnet-scan-snmp-route.php:58.

$e is used both as ‘local’ variable : it is local to the catch clause, and it is a blind variable in a foreach(). There is little overlap between the two occurrences, but one reader may wonder why the caught exception is shown later on.

try {
        $res = $Snmp->get_query(get_routing_table);
        // remove those not in subnet
        if (sizeof($res)>0) {
           // save for debug
           $debug[$d->hostname][$q] = $res;

           // save result
           $found[$d->id][$q] = $res;
        }
    } catch (Exception $e) {
       // save for debug
       $debug[$d->hostname][$q] = $res;
       $errors[] = $e->getMessage();
    }

// lots of code
// on line 132
    // print errors
    if (isset($errors)) {
        print <hr>;
        foreach ($errors as $e) {
            print $Result->show (warning, $e, false, false, true);
        }
    }

10.2.52.2. SuiteCrm

Catch Overwrite Variable, in modules/Emails/EmailUIAjax.php:1082.

$e starts as an Email(), in the ‘getMultipleMessagesFromSugar’ case, while a few lines later, in ‘refreshSugarFolders’, $e is now an exception. Breaks are in place, so both occurrences are separated, yet, one may wonder why an email is a warning, or a mail is a warning.

// On line 900, $e is a Email
        case getMultipleMessagesFromSugar:
            $GLOBALS['log']->debug(********** EMAIL 2.0 - Asynchronous - at: getMultipleMessagesFromSugar);
            if (isset($_REQUEST['uid']) && !empty($_REQUEST['uid'])) {
                $exIds = explode(,, $_REQUEST['uid']);
                $out = array();

                foreach ($exIds as $id) {
                    $e = new Email();
                    $e->retrieve($id);
                    $e->description_html = from_html($e->description_html);
                    $ie->email = $e;
                    $out[] = $ie->displayOneEmail($id, $_REQUEST['mbox']);
                }

                echo $json->encode($out);
            }

            break;


// lots of code
// on line 1082
        case refreshSugarFolders:
            try {
                $GLOBALS['log']->debug(********** EMAIL 2.0 - Asynchronous - at: refreshSugarFolders);
                $rootNode = new ExtNode('', '');
                $folderOpenState = $current_user->getPreference('folderOpenState', 'Emails');
                $folderOpenState = (empty($folderOpenState)) ?  : $folderOpenState;
                $ret = $email->et->folder->getUserFolders(
                    $rootNode,
                    sugar_unserialize($folderOpenState),
                    $current_user,
                    true
                );
                $out = $json->encode($ret);
                echo $out;
            } catch (SugarFolderEmptyException $e) {
                $GLOBALS['log']->warn($e);
                $out = $json->encode(array(
                    'message' => 'No folder selected warning message here...',
                ));
                echo $out;
            }
            break;

10.2.53. Deep Definitions

10.2.53.1. Dolphin

Deep Definitions, in wp-admin/includes/misc.php:74.

The ConstructHiddenValues function builds the ConstructHiddenSubValues function. Thus, ConstructHiddenValues can only be called once.

function ConstructHiddenValues($Values)
{
    /**
     *    Recursive function, processes multidimensional arrays
     *
     * @param string $Name  Full name of array, including all subarrays' names
     *
     * @param array  $Value Array of values, can be multidimensional
     *
     * @return string    Properly consctructed <input type="hidden"...> tags
     */
    function ConstructHiddenSubValues($Name, $Value)
    {
        if (is_array($Value)) {
            $Result = "";
            foreach ($Value as $KeyName => $SubValue) {
                $Result .= ConstructHiddenSubValues("{$Name}[{$KeyName}]", $SubValue);
            }
        } else // Exit recurse
        {
            $Result = "<input type=\"hidden\" name=\"" . htmlspecialchars($Name) . "\" value=\"" . htmlspecialchars($Value) . "\" />\n";
        }

        return $Result;
    }

    /* End of ConstructHiddenSubValues function */

    $Result = '';
    if (is_array($Values)) {
        foreach ($Values as $KeyName => $Value) {
            $Result .= ConstructHiddenSubValues($KeyName, $Value);
        }
    }

    return $Result;
}

10.2.54. Repeated print()

10.2.54.1. Edusoho

Repeated print(), in app/check.php:71.

All echo may be merged into one : do this by turning the ; and . into ‘,’, and removing the superfluous echo. Also, echo_style may be turned into a non-display function, returning the build style, rather than echoing it to the output.

echo PHP_EOL;
echo_style('title', 'Note');
echo '  The command console could use a different php.ini file'.PHP_EOL;
echo_style('title', '~~~~');
echo '  than the one used with your web server. To be on the'.PHP_EOL;
echo '      safe side, please check the requirements from your web'.PHP_EOL;
echo '      server using the ';
echo_style('yellow', 'web/config.php');
echo ' script.'.PHP_EOL;
echo PHP_EOL;

10.2.54.2. HuMo-Gen

Repeated print(), in menu.php:71.

Simply calling print once is better than three times. Here too, echo usage would reduce the amount of memory allocation due to concatenation prior display.

print '<input type=text name=quicksearch value=.$quicksearch. size=10 '.$pattern.' title=.__(Minimum:).$min_chars.__(characters).>';
                    print ' <input type=submit value=.__(Search).>';
            print </form>;

10.2.55. Objects Don’t Need References

10.2.55.1. Zencart

Objects Don’t Need References, in includes/library/illuminate/support/helpers.php:484.

No need for & operator when $class is only used for a method call.

/**
     * @param $class
     * @param $eventID
     * @param array $paramsArray
     */
    public function updateNotifyCheckoutflowFinishedManageSuccessOrderLinkEnd(&$class, $eventID, $paramsArray = array())
    {
        $class->getView()->getTplVarManager()->se('flag_show_order_link', false);
    }

10.2.55.2. XOOPS

Objects Don’t Need References, in htdocs/class/theme_blocks.phps:221.

Here, $template is modified, when its properties are modified. When only the properties are modified, or read, then & is not necessary.

public function buildBlock($xobject, &$template)
    {
        // The lame type workaround will change
        // bid is added temporarily as workaround for specific block manipulation
        $block = array(
            'id'      => $xobject->getVar('bid'),
            'module'  => $xobject->getVar('dirname'),
            'title'   => $xobject->getVar('title'),
            // 'name'        => strtolower( preg_replace( '/[^0-9a-zA-Z_]/', '', str_replace( ' ', '_', $xobject->getVar( 'name' ) ) ) ),
            'weight'  => $xobject->getVar('weight'),
            'lastmod' => $xobject->getVar('last_modified'));

        $bcachetime = (int)$xobject->getVar('bcachetime');
        if (empty($bcachetime)) {
            $template->caching = 0;
        } else {
            $template->caching        = 2;
            $template->cache_lifetime = $bcachetime;
        }
        $template->setCompileId($xobject->getVar('dirname', 'n'));
        $tplName = ($tplName = $xobject->getVar('template')) ? db:$tplName : 'db:system_block_dummy.tpl';
        $cacheid = $this->generateCacheId('blk_' . $xobject->getVar('bid'));
// more code to the end of the method

10.2.56. Lost References

10.2.56.1. WordPress

Lost References, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.57. Never Used Properties

10.2.57.1. WordPress

Never Used Properties, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.58. Useless Global

10.2.58.1. Zencart

Useless Global, in admin/includes/modules/newsletters/newsletter.php:25.

$_GET is always a global variable. There is no need to declare it global in any scope.

function choose_audience() {
      global $_GET;

10.2.58.2. HuMo-Gen

Useless Global, in admin/includes/modules/newsletters/newsletter.php:25.

It is hard to spot that $generY is useless, but this is the only occurrence where $generY is refered to as a global. It is not accessed anywhere else as a global (there are occurrences of $generY being an argument), and it is not even assigned within that function.

function calculate_ancestor($pers) {
global $db_functions, $reltext, $sexe, $sexe2, $spouse, $special_spouseY, $language, $ancestortext, $dutchtext, $selected_language, $spantext, $generY, $foundY_nr, $rel_arrayY;

10.2.59. Preprocessable

10.2.59.1. phpadsnew

Preprocessable, in phpAdsNew-2.0/adview.php:302.

Each call to chr() may be done before. First, chr() may be replace with the hexadecimal sequence “0x3B”; Secondly, 0x3b is a rather long replacement for a simple semi-colon. The whole pragraph could be stored in a separate file, for easier modifications.

echo chr(0x47).chr(0x49).chr(0x46).chr(0x38).chr(0x39).chr(0x61).chr(0x01).chr(0x00).
                 chr(0x01).chr(0x00).chr(0x80).chr(0x00).chr(0x00).chr(0x04).chr(0x02).chr(0x04).
                     chr(0x00).chr(0x00).chr(0x00).chr(0x21).chr(0xF9).chr(0x04).chr(0x01).chr(0x00).
                 chr(0x00).chr(0x00).chr(0x00).chr(0x2C).chr(0x00).chr(0x00).chr(0x00).chr(0x00).
                 chr(0x01).chr(0x00).chr(0x01).chr(0x00).chr(0x00).chr(0x02).chr(0x02).chr(0x44).
                 chr(0x01).chr(0x00).chr(0x3B);

10.2.60. Useless Unset

10.2.60.1. Tine20

Useless Unset, in tine20/Felamimail/Controller/Message.php:542.

$_rawContent is unset after being sent to the stream. The variable is a parameter, and will be freed at the end of the call of the method. No need to do it explicitly.

protected function _createMimePart($_rawContent, $_partStructure)
    {
        if (Tinebase_Core::isLogLevel(Zend_Log::TRACE)) Tinebase_Core::getLogger()->trace(__METHOD__ . '::' . __LINE__ . ' Content: ' . $_rawContent);

        $stream = fopen(php://temp, 'r+');
        fputs($stream, $_rawContent);
        rewind($stream);

        unset($_rawContent);
        //..... More code, no usage of $_rawContent
    }

10.2.60.2. Typo3

Useless Unset, in typo3/sysext/frontend/Classes/Page/PageRepository.php:708.

$row is unset under certain conditions : here, we can read it in the comments. Eventually, the $row will be returned, and turned into a NULL, by default. This will also create a notice in the logs. Here, the best would be to set a null value, instead of unsetting the variable.

public function getRecordOverlay($table, $row, $sys_language_content, $OLmode = '')
    {
//....  a lot more code, with usage of $row, and several unset($row)
//...... Reduced for simplicity
                    } else {
                        // When default language is displayed, we never want to return a record carrying
                        // another language!
                        if ($row[$GLOBALS['TCA'][$table]['ctrl']['languageField']] > 0) {
                            unset($row);
                        }
                    }
                }
            }
        }
        foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_page.php']['getRecordOverlay'] ?? [] as $className) {
            $hookObject = GeneralUtility::makeInstance($className);
            if (!$hookObject instanceof PageRepositoryGetRecordOverlayHookInterface) {
                throw new \UnexpectedValueException($className . ' must implement interface ' . PageRepositoryGetRecordOverlayHookInterface::class, 1269881659);
            }
            $hookObject->getRecordOverlay_postProcess($table, $row, $sys_language_content, $OLmode, $this);
        }
        return $row;
    }

10.2.61. Buried Assignation

10.2.61.1. XOOPS

Buried Assignation, in htdocs/image.php:170.

Classic iffectation : the condition also collects the needed value to process the drawing. This is very common in PHP, and the Yoda condition, with its constant on the left, shows that extra steps were taken to strengthen that piece of code.

if (0 < ($radius = $radii[2] * $q)) { // left bottom
        imagearc($workingImage, $radius - 1, $workingHeight - $radius, $radius * 2, $radius * 2, 90, 180, $alphaColor);
        imagefilltoborder($workingImage, 0, $workingHeight - 1, $alphaColor, $alphaColor);
    }

10.2.61.2. Mautic

Buried Assignation, in app/bundles/CoreBundle/Controller/ThemeController.php:47.

The setting of the variable $cancelled is fairly hidden here, with its extra operator !. The operator is here for the condition, as $cancelled needs the ‘cancellation’ state, while the condition needs the contrary. Note also that isset() could be moved out of this condition, and made the result easier to read.

$form        = $this->get('form.factory')->create('theme_upload', [], ['action' => $action]);

        if ($this->request->getMethod() == 'POST') {
            if (isset($form) && !$cancelled = $this->isFormCancelled($form)) {
                if ($this->isFormValid($form)) {
                    $fileData = $form['file']->getData();

10.2.62. No array_merge() In Loops

10.2.62.1. Tine20

No array_merge() In Loops, in tine20/Tinebase/User/Ldap.php:670.

Classic example of array_merge() in loop : here, the attributures should be collected in a local variable, and then merged in one operation, at the end. That includes the attributes provided before the loop, and the array provided after the loop. Note that the order of merge will be the same when merging than when collecting the arrays.

$attributes = array_values($this->_rowNameMapping);
        foreach ($this->_ldapPlugins as $plugin) {
            $attributes = array_merge($attributes, $plugin->getSupportedAttributes());
        }

        $attributes = array_merge($attributes, $this->_additionalLdapAttributesToFetch);

10.2.63. Useless Parenthesis

10.2.63.1. Mautic

Useless Parenthesis, in code/app/bundles/EmailBundle/Controller/AjaxController.php:85.

Parenthesis are useless around $progress[1], and around the division too.

$dataArray['percent'] = ($progress[1]) ? ceil(($progress[0] / $progress[1]) * 100) : 100;

10.2.63.2. Woocommerce

Useless Parenthesis, in code/app/bundles/EmailBundle/Controller/AjaxController.php:85.

Parenthesis are useless for calculating $discount_percent, as it is a divisition. Moreover, it is not needed with $discount, (float) applies to the next element, but it does make the expression more readable.

if ( wc_prices_include_tax() ) {
                            $discount_percent = ( wc_get_price_including_tax( $cart_item['data'] ) * $cart_item_qty ) / WC()->cart->subtotal;
                    } else {
                            $discount_percent = ( wc_get_price_excluding_tax( $cart_item['data'] ) * $cart_item_qty ) / WC()->cart->subtotal_ex_tax;
                    }
                    $discount = ( (float) $this->get_amount() * $discount_percent ) / $cart_item_qty;

10.2.64. Unresolved Instanceof

10.2.64.1. WordPress

Unresolved Instanceof, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

private function resolveTag($match)
    {
        $tagReflector = $this->createLinkOrSeeTagFromRegexMatch($match);
        if (!$tagReflector instanceof Tag\SeeTag && !$tagReflector instanceof Tag\LinkTag) {
            return $match;
        }

10.2.65. Use PHP Object API

10.2.65.1. WordPress

Use PHP Object API, in wp-includes/functions.php:2558.

Finfo has also a class, with the same name.

finfo_open(FILEINFO_MIME_TYPE)

10.2.65.2. PrestaShop

Use PHP Object API, in admin-dev/filemanager/include/utils.php:174.

transliterator_transliterate() has also a class named Transliterator

transliterator_transliterate('Accents-Any', $str)

10.2.65.3. SugarCrm

Use PHP Object API, in SugarCE-Full-6.5.26/include/database/MysqliManager.php:222.

Mysqli has also a class, with the same name.

mysqli_fetch_field_direct($result, $i)

10.2.66. Altering Foreach Without Reference

10.2.66.1. Contao

Altering Foreach Without Reference, in core-bundle/src/Resources/contao/classes/Theme.php:613.

$tmp[$kk] is &$vv.

foreach ($tmp as $kk=>$vv)
                                                            {
                                                                    // Do not use the FilesModel here – tables are locked!
                                                                    $objFile = $this->Database->prepare(SELECT uuid FROM tl_files WHERE path=?)
                                                                                                                      ->limit(1)
                                                                                                                      ->execute($this->customizeUploadPath($vv));

                                                                    $tmp[$kk] = $objFile->uuid;
                                                            }

10.2.66.2. WordPress

Altering Foreach Without Reference, in wp-admin/includes/misc.php:74.

$ids[$index] is &$rrid.

foreach($ids as $index => $rrid)
                {
                    if($rrid == $this->Id)
                    {
                        $ids[$index] = $_id;
                        $write = true;
                        break;
                    }
                }

10.2.67. Empty Instructions

10.2.67.1. Zurmo

Empty Instructions, in app/protected/core/widgets/MentionInput.php:84.

There is no need for a semi-colon after a if/then structure.

public function run()
        {
            $id = $this->getId();
            $additionalSettingsJs = showAvatars: . var_export($this->showAvatars, true) . ,;
            if ($this->classes)
            {
                $additionalSettingsJs .=  $this->classes . ',';
            };
            if ($this->templates)
            {
                $additionalSettingsJs .=  $this->templates;
            };

10.2.67.2. ThinkPHP

Empty Instructions, in ThinkPHP/Library/Vendor/Smarty/sysplugins/smarty_internal_configfileparser.php:83.

There is no need for a semi-colon after a class structure, unless it is an anonymous class.

class TPC_yyStackEntry
{
    public $stateno;       /* The state-number */
    public $major;         /* The major token value.  This is the code
                     ** number for the token at this stack level */
    public $minor; /* The user-supplied minor token value.  This
                     ** is the value of the token  */
};

10.2.68. Use Pathinfo

10.2.68.1. SuiteCrm

Use Pathinfo, in include/utils/file_utils.php:441.

Looking for the extension ? Use pathinfo() and PATHINFO_EXTENSION

$exp = explode('.', $filename);

10.2.69. Should Use Constants

10.2.69.1. Tine20

Should Use Constants, in tine20/Sales/Controller/Invoice.php:560.

True should be replaced by COUNT_RECURSIVE. The default one is COUNT_NORMAL.

count($billables, true)

10.2.70. No Parenthesis For Language Construct

10.2.70.1. Phpdocumentor

No Parenthesis For Language Construct, in src/Application/Renderer/Router/StandardRouter.php:55.

No need for parenthesis with require(). instanceof has a higher precedence than return anyway.

$this[] = new Rule(function ($node) { return ($node instanceof NamespaceDescriptor); }, $namespaceGenerator);

10.2.70.2. phpMyAdmin

No Parenthesis For Language Construct, in db_datadict.php:170.

Not only echo() doesn’t use any parenthesis, but this syntax gives the illusion that echo() only accepts one argument, while it actually accepts an arbitrary number of argument.

echo (($row['Null'] == 'NO') ? __('No') : __('Yes'))

10.2.71. No Hardcoded Port

10.2.71.1. WordPress

No Hardcoded Port, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.72. Use Constant As Arguments

10.2.72.1. Tikiwiki

Use Constant As Arguments, in lib/language/Language.php:112.

E_WARNING is a valid constant, but PHP documentation for trigger_error() explains that E_USER constants should be used.

trigger_error("Octal or hexadecimal string '" . $match[1] . "' not supported", E_WARNING)

10.2.72.2. shopware

Use Constant As Arguments, in engine/Shopware/Plugins/Default/Core/Debug/Components/EventCollector.php:106.

One example where code review reports errors where unit tests don’t : array_multisort actually requires sort order first (SORT_ASC or SORT_DESC), then sort flags (such as SORT_NUMERIC). Here, with SORT_DESC = 3 and SORT_NUMERIC = 1, PHP understands it as the coders expects it. The same error is repeated six times in the code.

array_multisort($order, SORT_NUMERIC, SORT_DESC, $this->results)

10.2.73. Assign Default To Properties

10.2.73.1. LiveZilla

Assign Default To Properties, in livezilla/_lib/functions.external.inc.php:174.

Flags may default to array() in the class definition. Filled array(), with keys and values, are also possible.

class OverlayChat
{
    public $Botmode;
    public $Human;
    public $HumanGeneral;
    public $RepollRequired;
    public $OperatorCount;
    public $Flags;
    public $LastMessageReceived;
    public $LastPostReceived;
    public $IsHumanChatAvailable;
    public $IsChatAvailable;
    public $ChatHTML;
    public $OverlayHTML;
    public $PostHTML;
    public $FullLoad;
    public $LanguageRequired = false;
    public $LastPoster;
    public $EyeCatcher;
    public $GroupBuilder;
    public $CurrentOperatorId;
    public $BotTitle;
    public $OperatorPostCount;
    public $PlaySound;
    public $SpeakingToHTML;
    public $SpeakingToAdded;
    public $Version = 1;

    public static $MaxPosts = 50;
    public static $Response;

    function __construct()
    {
        $this->Flags = array();
        VisitorChat::$Router = new ChatRouter();
    }

10.2.73.2. phpMyAdmin

Assign Default To Properties, in libraries/classes/Console.ph:55.

_isEnabled may default to true. It could also default to a class constant.

class Console
{
    /**
     * Whether to display anything
     *
     * @access private
     * @var bool
     */
    private $_isEnabled;

// some code ignored here
    /**
     * Creates a new class instance
     */
    public function __construct()
    {
        $this->_isEnabled = true;

10.2.74. Should Use Prepared Statement

10.2.74.1. Dolibarr

Should Use Prepared Statement, in htdocs/product/admin/price_rules.ph:76.

This code is well escaped, as the integer type cast will prevent any special chars to be used. Here, a prepared statement would apply a modern approach to securing this query.

$db->query("DELETE FROM " . MAIN_DB_PREFIX . "product_pricerules WHERE level = " . (int) $i)

10.2.74.2. Dolibarr

Should Use Prepared Statement, in htdocs/product/admin/price_rules.ph:76.

This code is well escaped, as the integer type cast will prevent any special chars to be used. Here, a prepared statement would apply a modern approach to securing this query.

$db->query("DELETE FROM " . MAIN_DB_PREFIX . "product_pricerules WHERE level = " . (int) $i)

10.2.75. No Hardcoded Ip

10.2.75.1. OpenEMR

No Hardcoded Ip, in wp-admin/includes/misc.php:74.

Although they are commented just above, the values provided here are suspicious.

// FTP parameters that you must customize.  If you are not sending
 // then set $FTP_SERVER to an empty string.
 //
 $FTP_SERVER = 192.168.0.30;
 $FTP_USER   = openemr;
 $FTP_PASS   = secret;
 $FTP_DIR    = ;

10.2.75.2. NextCloud

No Hardcoded Ip, in config/config.sample.php:1561.

Although they are documented as empty array, 3 values are provided as examples. They do not responds, at the time of writing, but they may.

/**
 * List of trusted proxy servers
 *
 * You may set this to an array containing a combination of
 * - IPv4 addresses, e.g. `192.168.2.123`
 * - IPv4 ranges in CIDR notation, e.g. `192.168.2.0/24`
 * - IPv6 addresses, e.g. `fd9e:21a7:a92c:2323::1`
 *
 * _(CIDR notation for IPv6 is currently work in progress and thus not
 * available as of yet)_
 *
 * When an incoming request's `REMOTE_ADDR` matches any of the IP addresses
 * specified here, it is assumed to be a proxy instead of a client. Thus, the
 * client IP will be read from the HTTP header specified in
 * `forwarded_for_headers` instead of from `REMOTE_ADDR`.
 *
 * So if you configure `trusted_proxies`, also consider setting
 * `forwarded_for_headers` which otherwise defaults to `HTTP_X_FORWARDED_FOR`
 * (the `X-Forwarded-For` header).
 *
 * Defaults to an empty array.
 */
'trusted_proxies' => array('203.0.113.45', '198.51.100.128', '192.168.2.0/24'),

10.2.76. Echo With Concat

10.2.76.1. Phpdocumentor

Echo With Concat, in src/phpDocumentor/Bootstrap.php:76.

Simply replace the dot by a comma.

echo 'PROFILING ENABLED' . PHP_EOL

10.2.76.2. TeamPass

Echo With Concat, in includes/libraries/Authentication/Yubico/PEAR.php:162.

This is less obvious, but turning print to echo, and the double-quoted string to single quoted string will yield the same optimisation.

print "PEAR constructor called, class=$classname\n";

10.2.77. Else If Versus Elseif

10.2.77.1. TeamPass

Else If Versus Elseif, in items.php:819.

This code could be turned into a switch() structure.

if ($field[3] === 'text') {
                echo '
                        <input type=text id=edit_field_.$field[0]._.$elem[0]. class=edit_item_field input_text text ui-widget-content ui-corner-all size=40 data-field-type=.$field[3]. data-field-masked=.$field[4]. data-field-is-mandatory=.$field[5]. data-template-id=.$templateID.>';
            } else if ($field[3] === 'textarea') {
                echo '
                        <textarea id=edit_field_.$field[0]._.$elem[0]. class=edit_item_field input_text text ui-widget-content ui-corner-all colums=40 rows=5 data-field-type=.$field["3"]. data-field-masked=.$field[4]. data-field-is-mandatory=.$field[5]. data-template-id=.$templateID.></textarea>';
            }

10.2.77.2. Phpdocumentor

Else If Versus Elseif, in src/phpDocumentor/Plugin/Core/Transformer/Writer/Xsl.php:112.

The first then block is long and complex. The else block, on the other hand, only contains a single if/then/else. Both conditions are distinct at first sight, so a if / elseif / then structure would be the best.

if ($transformation->getQuery() !== '') {
/** Long then block **/
        } else {
            if (substr($transformation->getArtifact(), 0, 1) == '$') {
                // not a file, it must become a variable!
                $variable_name = substr($transformation->getArtifact(), 1);
                $this->xsl_variables[$variable_name] = $proc->transformToXml($structure);
            } else {
                $relativeFileName = substr($artifact, strlen($transformation->getTransformer()->getTarget()) + 1);
                $proc->setParameter('', 'root', str_repeat('../', substr_count($relativeFileName, '/')));

                $this->writeToFile($artifact, $proc, $structure);
            }
        }

10.2.78. Could Be Static

10.2.78.1. Dolphin

Could Be Static, in inc/utils.inc.php:673.

Dolphin pro relies on HTMLPurifier to handle cleaning of values : it is used to prevent xss threat. In this method, oHtmlPurifier is first checked, and if needed, created. Since creation is long and costly, it is only created once. Once the object is created, it is stored as a global to be accessible at the next call of the method. In fact, oHtmlPurifier is never used outside this method, so it could be turned into a ‘static’ variable, and prevent other methods to modify it. This is a typical example of variable that could be static instead of global.

function clear_xss($val)
{
    // HTML Purifier plugin
    global $oHtmlPurifier;
    if (!isset($oHtmlPurifier) && !$GLOBALS['logged']['admin']) {

        require_once(BX_DIRECTORY_PATH_PLUGINS . 'htmlpurifier/HTMLPurifier.standalone.php');

/..../

        $oHtmlPurifier = new HTMLPurifier($oConfig);
    }

    if (!$GLOBALS['logged']['admin']) {
        $val = $oHtmlPurifier->purify($val);
    }

    $oZ = new BxDolAlerts('system', 'clear_xss', 0, 0,
        array('oHtmlPurifier' => $oHtmlPurifier, 'return_data' => &$val));
    $oZ->alert();

    return $val;
}

10.2.78.2. Contao

Could Be Static, in system/helper/functions.php:184.

$arrScanCache is a typical cache variables. It is set as global for persistence between calls. If it contains an already stored answer, it is returned immediately. If it is not set yet, it is then filled with a value, and later reused. This global could be turned into static, and avoid pollution of global space.

function scan($strFolder, $blnUncached=false)
{
    global $arrScanCache;

    // Add a trailing slash
    if (substr($strFolder, -1, 1) != '/')
    {
            $strFolder .= '/';
    }

    // Load from cache
    if (!$blnUncached && isset($arrScanCache[$strFolder]))
    {
            return $arrScanCache[$strFolder];
    }
    $arrReturn = array();

    // Scan directory
    foreach (scandir($strFolder) as $strFile)
    {
            if ($strFile == '.' || $strFile == '..')
            {
                    continue;
            }

            $arrReturn[] = $strFile;
    }

    // Cache the result
    if (!$blnUncached)
    {
            $arrScanCache[$strFolder] = $arrReturn;
    }

    return $arrReturn;
}

10.2.79. Could Use Short Assignation

10.2.79.1. ChurchCRM

Could Use Short Assignation, in src/ChurchCRM/utils/GeoUtils.php:74.

Sometimes, the variable is on the other side of the operator.

$distance = 0.6213712 * $distance;

10.2.79.2. Thelia

Could Use Short Assignation, in local/modules/Tinymce/Resources/js/tinymce/filemanager/include/utils.php:70.

/= is rare, but it definitely could be used here.

$size = $size / 1024;

10.2.80. Pre-increment

10.2.80.1. ExpressionEngine

Pre-increment, in system/ee/EllisLab/ExpressionEngine/Controller/Utilities/Communicate.php:650.

Using preincrement in for() loops is safe and straightforward.

for ($x = 0; $x < $number_to_send; $x++)
            {
                    $email_address = array_shift($recipient_array);

                    if ( ! $this->deliverEmail($email, $email_address))
                    {
                            $email->delete();

                            $debug_msg = ee()->email->print_debugger(array());

                            show_error(lang('error_sending_email').BR.BR.$debug_msg);
                    }
                    $email->total_sent++;
            }

10.2.80.2. Traq

Pre-increment, in src/Controllers/Tickets.php:84.

$this->currentProject->next_ticket_id value is ignored by the code. It may be turned into a preincrement.

TimelineModel::newTicketEvent($this->currentUser, $ticket)->save();

            $this->currentProject->next_ticket_id++;
            $this->currentProject->save();

10.2.81. Indices Are Int Or String

10.2.81.1. Zencart

Indices Are Int Or String, in includes/modules/payment/paypaldp.php:2523.

All those strings ends up as integers.

// Build Currency format table
    $curFormat = Array();
    $curFormat[036]=2;
    $curFormat[124]=2;
    $curFormat[203]=2;
    $curFormat[208]=2;
    $curFormat[348]=2;
    $curFormat[392]=0;
    $curFormat[554]=2;
    $curFormat[578]=2;
    $curFormat[702]=2;
    $curFormat[752]=2;
    $curFormat[756]=2;
    $curFormat[826]=2;
    $curFormat[840]=2;
    $curFormat[978]=2;
    $curFormat[985]=2;

10.2.81.2. Mautic

Indices Are Int Or String, in app/bundles/CoreBundle/Entity/CommonRepository.php:315.

$baseCols has 1 and 0 (respectively) for index.

foreach ($metadata->getAssociationMappings() as $field => $association) {
                    if (in_array($association['type'], [ClassMetadataInfo::ONE_TO_ONE, ClassMetadataInfo::MANY_TO_ONE])) {
                        $baseCols[true][$entityClass][]  = $association['joinColumns'][0]['name'];
                        $baseCols[false][$entityClass][] = $field;
                    }
                }

10.2.82. Should Typecast

10.2.82.1. xataface

Should Typecast, in Dataface/Relationship.php:1612.

This is an exact example. A little further, the same applies to intval($max))

intval($min);

10.2.82.2. OpenConf

Should Typecast, in author/upload.php:62.

This is another exact example.

intval($_POST['pid']);

10.2.83. No Direct Usage

10.2.83.1. Edusoho

No Direct Usage, in edusoho/src/AppBundle/Controller/Admin/FinanceSettingController.php:107.

Glob() returns false, in case of error. It returns an empty array in case everything is fine, but nothing was found. In case of error, array_map() will stop the script.

array_map('unlink', glob($dir.'/MP_verify_*.txt'));

10.2.83.2. XOOPS

No Direct Usage, in htdocs/Frameworks/moduleclasses/moduleadmin/moduleadmin.php:585.

Although the file is readable, file() may return false in case of failure. On the other hand, implode doesn’t accept boolean values.

$file = XOOPS_ROOT_PATH . /modules/{$module_dir}/docs/changelog.txt;
            if ( is_readable( $file ) ) {
                $ret .= implode( '<br>', file( $file ) ) . \n;
            }

10.2.84. Useless Brackets

10.2.84.1. ChurchCRM

Useless Brackets, in src/Menu.php:72.

Difficut to guess what was before the block here. It doesn’t have any usage for control flow.

$new_row = false;
        $count_people = 0;

        {
            foreach ($peopleWithBirthDays as $peopleWithBirthDay) {
                if ($new_row == false) {
                    ?>

                    <div class=row>
                <?php
                    $new_row = true;
                } ?>
                <div class=col-sm-3>

10.2.84.2. Piwigo

Useless Brackets, in picture.php:342.

There is no need for block braces with case. In fact, it does give a false sense of break, while the case will still fall over to the next one.

case 'rate' :
    {
      include_once(PHPWG_ROOT_PATH.'include/functions_rate.inc.php');
      rate_picture($page['image_id'], $_POST['rate']);
      redirect($url_self);
    }

10.2.85. preg_replace With Option e

10.2.85.1. Edusoho

preg_replace With Option e, in vendor_user/uc_client/lib/uccode.class.php:32.

This call extract text between [code] tags, then process it with $this->codedisp() and nest it again in the original string. preg_replace_callback() is a drop-in replacement for this piece of code.

$message = preg_replace("/\s*\[code\](.+?)\[\/code\]\s*/ies", "$this->codedisp('\1')", $message);

10.2.86. eval() Without Try

10.2.86.1. FuelCMS

eval() Without Try, in fuel/modules/fuel/controllers/Blocks.php:268.

The @ will prevent any error, while the try/catch allows the processing of certain types of error, namely the Fatal ones.

@eval($_name_var_eval)

10.2.86.2. ExpressionEngine

eval() Without Try, in system/ee/EllisLab/Addons/member/mod.member_memberlist.php:637.

$cond is build from values extracted from the $fields array. Although it is probably reasonably safe, a try/catch here will collect any unexpected situation cleaningly.

elseif (isset($fields[$val['3']]))
                                    {
                                            if (array_key_exists('m_field_id_'.$fields[$val['3']], $row))
                                            {
                                                    $v = $row['m_field_id_'.$fields[$val['3']]];

                                                    $lcond = str_replace($val['3'], "$v", $lcond);
                                                    $cond = $lcond.' '.$rcond;
                                                    $cond = str_replace("\|", "|", $cond);

                                                    eval("$result = ".$cond.";");

10.2.87. Relay Function

10.2.87.1. TeamPass

Relay Function, in includes/libraries/Goodby/CSV/Import/Standard/Interpreter.php:88.

This example puts actually a name on the events : this method ‘delegate’ and it does it in the smallest amount of possible work, being given all the arguments.

/**
     * delegate to observer
     *
     * @param $observer
     * @param $line
     */
    private function delegate($observer, $line)
    {
        call_user_func($observer, $line);
    }

10.2.87.2. SPIP

Relay Function, in ecrire/inc/json.php:73.

var2js() acts as an alternative for json_encode(). Yet, it used to be directly called by the framework’s code and difficult to change. With the advent of json_encode, the native function has been used, and even, a compatibility tool was set up. Thus, the relay function.

if (!function_exists('json_encode')) {
    function json_encode($v) {
            return var2js($v);
    }
}

10.2.88. Silently Cast Integer

10.2.88.1. MediaWiki

Silently Cast Integer, in includes/debug/logger/monolog/AvroFormatter.php:167.

Too many ff in the masks.

private function encodeLong( $id ) {
            $high   = ( $id & 0xffffffff00000000 ) >> 32;
            $low    = $id & 0x00000000ffffffff;
            return pack( 'NN', $high, $low );
    }

10.2.89. Timestamp Difference

10.2.89.1. Zurmo

Timestamp Difference, in app/protected/modules/import/jobs/ImportCleanupJob.php:73.

This is wrong twice a year, in countries that has day-ligth saving time. One of the weeks will be too short, and the other will be too long.

/**
         * Get all imports where the modifiedDateTime was more than 1 week ago.  Then
         * delete the imports.
         * (non-PHPdoc)
         * @see BaseJob::run()
         */
        public function run()
        {
            $oneWeekAgoTimeStamp = DateTimeUtil::convertTimestampToDbFormatDateTime(time() - 60 * 60 *24 * 7);

10.2.89.2. shopware

Timestamp Difference, in engine/Shopware/Controllers/Backend/Newsletter.php:150.

When daylight saving strike, the email may suddenly be locked for 1 hour minus 30 seconds ago. The lock will be set for the rest of the hour, until the server catch up.

// Check lock time. Add a buffer of 30 seconds to the lock time (default request time)
            if (!empty($mailing['locked']) && strtotime($mailing['locked']) > time() - 30) {
                echo "Current mail: '" . $subjectCurrentMailing . "'\n";
                echo "Wait " . (strtotime($mailing['locked']) + 30 - time()) . " seconds ...\n";
                return;
            }

10.2.90. Unused Arguments

10.2.90.1. ThinkPHP

Unused Arguments, in ThinkPHP/Library/Behavior/AgentCheckBehavior.class.php:18.

$params are requested, but never used. The method is not overloading another one, as the class doesn’t extends anything. $params is unused.

class AgentCheckBehavior
{
    public function run(&$params)
    {
        // 代理访问检测
        $limitProxyVisit = C('LIMIT_PROXY_VISIT', null, true);
        if ($limitProxyVisit && ($_SERVER['HTTP_X_FORWARDED_FOR'] || $_SERVER['HTTP_VIA'] || $_SERVER['HTTP_PROXY_CONNECTION'] || $_SERVER['HTTP_USER_AGENT_VIA'])) {
            // 禁止代理访问
            exit('Access Denied');
        }
    }
}

10.2.90.2. phpMyAdmin

Unused Arguments, in libraries/classes/Display/Results.php:1985.

Although $column_index is documented, it is not found in the rest of the (long) body of the function. It might have been refactored into $sorted_column_index.

/**
     * Prepare parameters and html for sorted table header fields
     *
     * @param array    $sort_expression             sort expression
     * @param array    $sort_expression_nodirection sort expression without direction
     * @param string   $sort_tbl                    The name of the table to which
     *                                             the current column belongs to
     * @param string   $name_to_use_in_sort         The current column under
     *                                             consideration
     * @param array    $sort_direction              sort direction
     * @param stdClass $fields_meta                 set of field properties
     * @param integer  $column_index                The index number to current column
     *
     * @return  array   3 element array - $single_sort_order, $sort_order, $order_img
     *
     * @access  private
     *
     * @see     _getOrderLinkAndSortedHeaderHtml()
     */
    private function _getSingleAndMultiSortUrls(
        array $sort_expression,
        array $sort_expression_nodirection,
        $sort_tbl,
        $name_to_use_in_sort,
        array $sort_direction,
        $fields_meta,
        $column_index
    ) {
    /**/
        // find the sorted column index in row result
        // (this might be a multi-table query)
        $sorted_column_index = false;
    /**/
    }

10.2.91. Switch To Switch

10.2.91.1. Thelia

Switch To Switch, in core/lib/Thelia/Controller/Admin/TranslationsController.php:100.

The two first comparison may be turned into a case, and the last one could be default, or default with a check on empty().

if($modulePart == 'core') { /**/ } elseif($modulePart == 'admin-includes') { /**/ } elseif(!empty($modulePart)) { /**/ }

10.2.91.2. XOOPS

Switch To Switch, in htdocs/search.php:74.

Here, converting this structure to switch requires to drop the === usage. Also, no default usage here.

if($action === 'results') { /**/ } elseif($action === 'showall') { /**/ } elseif($action === 'showallbyuser') { /**/ }

10.2.92. Wrong Parameter Type

10.2.92.1. Zencart

Wrong Parameter Type, in admin/includes/header.php:180.

setlocale() may be called with null or ‘’ (empty string), and will set values from the environnement. When called with “0” (the string), it only reports the current setting. Using an integer is probably undocumented behavior, and falls back to the zero string.

$loc = setlocale(LC_TIME, 0);
        if ($loc !== FALSE) echo ' - ' . $loc; //what is the locale in use?

10.2.93. Wrong fopen() Mode

10.2.93.1. Tikiwiki

Wrong fopen() Mode, in lib/tikilib.php:6777.

This fopen() mode doesn’t exists. Use ‘w’ instead.

fopen('php://temp', 'rw');

10.2.93.2. HuMo-Gen

Wrong fopen() Mode, in include/phprtflite/lib/PHPRtfLite/StreamOutput.php:77.

This fopen() mode doesn’t exists. Use ‘w’ instead.

fopen($this->_filename, 'wr', false)

10.2.94. Use random_int()

10.2.94.1. Thelia

Use random_int(), in core/lib/Thelia/Tools/TokenProvider.php:151.

The whole function may be replaced by random_int(), as it generates random tokens. This needs an extra layer of hashing, to get a long and string results.

/**
     * @return string
     */
    protected static function getComplexRandom()
    {
        $firstValue = (float) (mt_rand(1, 0xFFFF) * rand(1, 0x10001));
        $secondValues = (float) (rand(1, 0xFFFF) * mt_rand(1, 0x10001));

        return microtime() . ceil($firstValue / $secondValues) . uniqid();
    }

10.2.94.2. FuelCMS

Use random_int(), in fuel/modules/fuel/libraries/Fuel.php:235.

Security tokens should be build with a CSPRNG source. uniqid() is based on time, and though it changes anytime (sic), it is easy to guess. Those days, it looks like ‘5b1262e74dbb9’;

$this->installer->change_config('config', '$config[\'encryption_key\'] = \'\';', '$config[\'encryption_key\'] = \''.md5(uniqid()).'\';');

10.2.95. Already Parents Interface

10.2.95.1. WordPress

Already Parents Interface, in src/Phinx/Db/Adapter/AbstractAdapter.php:41.

SqlServerAdapter extends PdoAdapter, PdoAdapter extends AbstractAdapter. The first and the last both implements AdapterInterface. Only one is needed.

/**
 * Base Abstract Database Adapter.
 */
abstract class AbstractAdapter implements AdapterInterface
{

/// In the src/src/Phinx/Db/Adapter/SqlServerAdapter.php, line 45
/**
 * Phinx SqlServer Adapter.
 *
 */
class SqlServerAdapter extends PdoAdapter implements AdapterInterface
{

10.2.95.2. Thelia

Already Parents Interface, in core/lib/Thelia/Core/Template/Loop/BaseSpecificModule.php:35.

PropelSearchLoopInterface is implemented by both BaseSpecificModule and Payment

abstract class BaseSpecificModule extends BaseI18nLoop implements PropelSearchLoopInterface

/* in file  core/lib/Thelia/Core/Template/Loop/Payment.php, line 28 */

class Payment extends BaseSpecificModule implements PropelSearchLoopInterface

10.2.96. Invalid Class Name

10.2.96.1. WordPress

Invalid Class Name, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.97. Ternary In Concat

10.2.97.1. TeamPass

Ternary In Concat, in includes/libraries/protect/AntiXSS/UTF8.php:5409.

The concatenations in the initial comparison are disguised casting. When $str2 is empty too, the ternary operator yields a 0, leading to a systematic failure.

$str1 . '' === $str2 . '' ? 0 : strnatcmp(self::strtonatfold($str1), self::strtonatfold($str2))

10.2.98. No Hardcoded Hash

10.2.98.1. shopware

No Hardcoded Hash, in engine/Shopware/Models/Document/Data/OrderData.php:254.

This is actually a hashed hardcoded password. As the file explains, this is a demo order, for populating the database when in demo mode, so this is fine. We also learn that the password are securily sorted here. It may also be advised to avoid hardcoding this password, as any demo shop has the same user credential : it is the first to be tried when a demo installation is found.

'_userID' => '3',
    '_user' => new ArrayObject([
            'id' => '3',
            'password' => '$2y$10$GAGAC6.1kMRvN4RRcLrYleDx.EfWhHcW./cmoOQg11sjFUY73SO.C',
            'encoder' => 'bcrypt',
            'email' => 'demo@shopware.com',
            'customernumber' => '20005',

10.2.98.2. SugarCrm

No Hardcoded Hash, in SugarCE-Full-6.5.26/include/Smarty/Smarty.class.php:460.

The MD5(‘Smarty’) is hardcoded in the properties. This property is not used in the class, but in parts of the code, when a unique delimiter is needed.

/**
     * md5 checksum of the string 'Smarty'
     *
     * @var string
     */
    var $_smarty_md5           = 'f8d698aea36fcbead2b9d5359ffca76f';

10.2.99. Identical Conditions

10.2.99.1. WordPress

Identical Conditions, in wp-admin/theme-editor.php:247.

The condition checks first if $has_templates or $theme->parent(), and one of the two is sufficient to be valid. Then, it checks again that $theme->parent() is activated with &&. This condition may be reduced by calling $theme->parent(), as $has_template is unused here.

<?php if ( ( $has_templates || $theme->parent() ) && $theme->parent() ) : ?>

10.2.99.2. Dolibarr

Identical Conditions, in htdocs/core/lib/files.lib.php:2052.

Better check twice that $modulepart is really ‘apercusupplier_invoice’.

$modulepart == 'apercusupplier_invoice' || $modulepart == 'apercusupplier_invoice'

10.2.99.3. Mautic

Identical Conditions, in app/bundles/CoreBundle/Views/Standard/list.html.php:47.

When the line is long, it tends to be more and more difficult to review the values. Here, one of the two first is too many.

!empty($permissions[$permissionBase . ':deleteown']) || !empty($permissions[$permissionBase . ':deleteown']) || !empty($permissions[$permissionBase . ':delete'])

10.2.100. No Choice

10.2.100.1. NextCloud

No Choice, in build/integration/features/bootstrap/FilesDropContext.php:71.

Token is checked, but processed in the same way each time. This actual check is done twice, in the same class, in the method droppingFileWith().

public function creatingFolderInDrop($folder) {
            $client = new Client();
            $options = [];
            if (count($this->lastShareData->data->element) > 0){
                    $token = $this->lastShareData->data[0]->token;
            } else {
                    $token = $this->lastShareData->data[0]->token;
            }
            $base = substr($this->baseUrl, 0, -4);
            $fullUrl = $base . '/public.php/webdav/' . $folder;

            $options['auth'] = [$token, ''];

10.2.100.2. Zencart

No Choice, in admin/includes/functions/html_output.php:179.

At least, it always choose the most secure way : use SSL.

if ($usessl) {
        $form .= zen_href_link($action, $parameters, 'NONSSL');
      } else {
        $form .= zen_href_link($action, $parameters, 'NONSSL');
      }

10.2.101. Logical Mistakes

10.2.101.1. Dolibarr

Logical Mistakes, in htdocs/core/lib/admin.lib.php:1165.

This expression is always true. When $nbtabsql is $nbtablib, the left part is true; When $nbtabsql is $nbtabsqlsort, the right part is true; When any other value is provided, both operands are true.

$nbtablib != $nbtabsql || $nbtabsql != $nbtabsqlsort

10.2.101.2. Cleverstyle

Logical Mistakes, in modules/HybridAuth/Hybrid/Providers/DigitalOcean.php:123.

This expression is always false. When $data->account->email_verified is true, the right part is false; When $data->account->email_verified is $data->account->email, the right part is false; The only viable solution is to have ` $data->account->email`true : this is may be the intend it, though it is not easy to understand.

TRUE == $data->account->email_verified and $data->account->email == $data->account->email_verified

10.2.102. Return True False

10.2.102.1. Mautic

Return True False, in app/bundles/LeadBundle/Model/ListModel.php:125.

$isNew could be a typecast.

$isNew = ($entity->getId()) ? false : true;

10.2.102.2. FuelCMS

Return True False, in fuel/modules/fuel/helpers/validator_helper.php:254.

If/then is a lot of code to produce a boolean.

function length_min($str, $limit = 1)
    {
            if (strlen(strval($str)) < $limit)
            {
                    return FALSE;
            }
            else
            {
                    return TRUE;
            }
    }

10.2.103. Useless Switch

10.2.103.1. Phpdocumentor

Useless Switch, in fuel/modules/fuel/libraries/Inspection.php:349.

This method parses comments. In fact, comments are represented by other tokens, which may be added or removed at time while coding.

public function parse_comments($code)
    {
            $comments = array();
            $tokens = token_get_all($code);

            foreach($tokens as $token)
            {
                    switch($token[0])
                    {
                            case T_DOC_COMMENT:
                                    $comments[] = $token[1];
                                    break;
                }
            }
            return $comments;

    }

10.2.103.2. Dolphin

Useless Switch, in Dolphin-v.7.3.5/inc/classes/BxDolModuleDb.php:34.

$aParams is an argument : this code looks like the switch is reserved for future use.

function getModulesBy($aParams = array())
    {
            $sMethod = 'getAll';
        $sPostfix = $sWhereClause = "";

        $sOrderClause = "ORDER BY `title`";
        switch($aParams['type']) {
            case 'path':
                    $sMethod = 'getRow';
                $sPostfix .= '_path';
                $sWhereClause .= "AND `path`='" . $aParams['value'] . "'";
                break;
        }

10.2.104. Could Use __DIR__

10.2.104.1. Woocommerce

Could Use __DIR__, in includes/class-wc-api.php:162.

All the 120 occurrences use dirname( __FILE__ ), and could be upgraded to __DIR__ if backward compatibility to PHP 5.2 is not critical.

private function rest_api_includes() {
            // Exception handler.
            include_once dirname( __FILE__ ) . '/api/class-wc-rest-exception.php';

            // Authentication.
            include_once dirname( __FILE__ ) . '/api/class-wc-rest-authentication.php';

10.2.104.2. Piwigo

Could Use __DIR__, in include/random_compat/random.php:50.

dirname( __FILE__ ) is cached into $RandomCompatDIR, then reused three times. Using __DIR__ would save that detour.

$RandomCompatDIR = dirname(__FILE__);

    require_once $RandomCompatDIR.'/byte_safe_strings.php';
    require_once $RandomCompatDIR.'/cast_to_int.php';
    require_once $RandomCompatDIR.'/error_polyfill.php';

10.2.105. Should Use Coalesce

10.2.105.1. ChurchCRM

Should Use Coalesce, in src/ChurchCRM/Service/FinancialService.php:597.

ChurchCRM features 5 old style ternary operators, which are all in this SQL query. ChurchCRM requires PHP 7.0, so a simple code review could remove them all.

$sSQL = "INSERT INTO pledge_plg
                    (plg_famID,
                    plg_FYID,
                    plg_date,
                    plg_amount,
                    plg_schedule,
                    plg_method,
                    plg_comment,
                    plg_DateLastEdited,
                    plg_EditedBy,
                    plg_PledgeOrPayment,
                    plg_fundID,
                    plg_depID,
                    plg_CheckNo,
                    plg_scanString,
                    plg_aut_ID,
                    plg_NonDeductible,
                    plg_GroupKey)
                    VALUES ('".
          $payment->FamilyID."','".
          $payment->FYID."','".
          $payment->Date."','".
          $Fund->Amount."','".
          (isset($payment->schedule) ? $payment->schedule : 'NULL')."','".
          $payment->iMethod."','".
          $Fund->Comment."','".
          date('YmdHis')."',".
          $_SESSION['user']->getId().",'".
          $payment->type."',".
          $Fund->FundID.','.
          $payment->DepositID.','.
          (isset($payment->iCheckNo) ? $payment->iCheckNo : 'NULL').",'".
          (isset($payment->tScanString) ? $payment->tScanString : 'NULL')."','".
          (isset($payment->iAutID) ? $payment->iAutID : 'NULL')."','".
          (isset($Fund->NonDeductible) ? $Fund->NonDeductible : 'NULL')."','".
          $sGroupKey."')";

10.2.105.2. Cleverstyle

Should Use Coalesce, in modules/Feedback/index.php:37.

Cleverstyle nests ternary operators when selecting default values. Here, moving some of them to ?? will reduce the code complexity and make it more readable. Cleverstyle requires PHP 7.0 or more recent.

$Page->content(
    h::{'cs-form form'}(
            h::{'section.cs-feedback-form article'}(
                    h::{'header h2.cs-text-center'}($L->Feedback).
                    h::{'table.cs-table[center] tr| td'}(
                            [
                                    h::{'cs-input-text input[name=name][required]'}(
                                            [
                                                    'placeholder' => $L->feedback_name,
                                                    'value'       => $User->user() ? $User->username() : (isset($_POST['name']) ? $_POST['name'] : '')
                                            ]
                                    ),
                                    h::{'cs-input-text input[type=email][name=email][required]'}(
                                            [
                                                    'placeholder' => $L->feedback_email,
                                                    'value'       => $User->user() ? $User->email : (isset($_POST['email']) ? $_POST['email'] : '')
                                            ]
                                    ),
                                    h::{'cs-textarea[autosize] textarea[name=text][required]'}(
                                            [
                                                    'placeholder' => $L->feedback_text,
                                                    'value'       => isset($_POST['text']) ? $_POST['text'] : ''
                                            ]
                                    ),
                                    h::{'cs-button button[type=submit]'}($L->feedback_send)
                            ]
                    )
            )
    )
);

10.2.106. If With Same Conditions

10.2.106.1. phpMyAdmin

If With Same Conditions, in libraries/classes/Response.php:345.

The first test on $this->_isSuccess settles the situation with _JSON. Then, a second check is made. Both could be merged, also the second one is fairly long (not shown).

if ($this->_isSuccess) {
            $this->_JSON['success'] = true;
        } else {
            $this->_JSON['success'] = false;
            $this->_JSON['error']   = $this->_JSON['message'];
            unset($this->_JSON['message']);
        }

        if ($this->_isSuccess) {

10.2.106.2. Phpdocumentor

If With Same Conditions, in src/phpDocumentor/Transformer/Command/Project/TransformCommand.php:239.

$templates is extracted from $input. If it is empty, a second source is polled. Finally, if nothing has worked, a default value is used (‘clean’). In this case, each attempt is an alternative solution to the previous failing call. The second test could be reported on $templatesFromConfig, and not $templates.

$templates = $input->getOption('template');
        if (!$templates) {
            /** @var Template[] $templatesFromConfig */
            $templatesFromConfig = $configurationHelper->getConfigValueFromPath('transformations/templates');
            foreach ($templatesFromConfig as $template) {
                $templates[] = $template->getName();
            }
        }

        if (!$templates) {
            $templates = array('clean');
        }

10.2.107. Throw Functioncall

10.2.107.1. SugarCrm

Throw Functioncall, in include/externalAPI/cmis_repository_wrapper.php:918.

SugarCRM uses exceptions to fill work in progress. Here, we recognize a forgotten ‘new’ that makes throw call a function named ‘Exception’. This fails with a Fatal Error, and doesn’t issue the right messsage. The same error had propgated in the code by copy and paste : it is available 17 times in that same file.

function getContentChanges()
    {
        throw Exception("Not Implemented");
    }

10.2.107.2. Zurmo

Throw Functioncall, in app/protected/modules/gamification/rules/collections/GameCollectionRules.php:66.

Other part of the code actually instantiate the exception before throwing it.

abstract class GameCollectionRules
    {
        /**
         * @return string
         * @throws NotImplementedException - Implement in children classes
         */
        public static function getType()
        {
            throw NotImplementedException();
        }

10.2.108. Use Instanceof

10.2.108.1. TeamPass

Use Instanceof, in includes/libraries/Database/Meekrodb/db.class.php:506.

In this code, is_object() and instanceof have the same basic : they both check that $ts is an object. In fact, instanceof is more precise, and give more information about the variable.

protected function parseTS($ts) {
    if (is_string($ts)) return date('Y-m-d H:i:s', strtotime($ts));
    else if (is_object($ts) && ($ts instanceof DateTime)) return $ts->format('Y-m-d H:i:s');
  }

10.2.108.2. Zencart

Use Instanceof, in includes/modules/payment/firstdata_hco.php:104.

In this code, is_object() is used to check the status of the order. Possibly, $order is false or null in case of incompatible status. Yet, when $object is an object, and in particular being a global that may be assigned anywhere else in the code, it seems that the method ‘update_status’ is magically always available. Here, using instance of to make sure that $order is an ‘paypal’ class, or a ‘storepickup’ or any of the payment class.

function __construct() {
    global $order;

    // more lines, no mention of $order
    if (is_object($order)) $this->update_status();

    // more code
}

10.2.109. Always Positive Comparison

10.2.109.1. Magento

Always Positive Comparison, in app/code/core/Mage/Dataflow/Model/Profile.php:85.

strlen(($actiosXML) will never be negative, and hence, is always false. This exception is never thrown.

if (strlen($actionsXML) < 0 &&
        @simplexml_load_string('<data>' . $actionsXML . '</data>', null, LIBXML_NOERROR) === false) {
            Mage::throwException(Mage::helper('dataflow')->__("Actions XML is not valid."));
        }

10.2.110. Empty Blocks

10.2.110.1. Cleverstyle

Empty Blocks, in modules/Blogs/api/Controller.php:44.

Else is empty, but commented.

public static function posts_get ($Request) {
            $id = $Request->route_ids(0);
            if ($id) {
                    $post = Posts::instance()->get($id);
                    if (!$post) {
                            throw new ExitException(404);
                    }
                    return $post;
            } else {
                    // TODO: implement latest posts
            }
    }

10.2.110.2. PhpIPAM

Empty Blocks, in wp-admin/includes/misc.php:74.

The then block is empty and commented : yet, it may have been clearer to make the condition != and omitted the whole empty block.

/* checks */
if($_POST['action'] == delete) {
    # no cecks
}
else {
    # remove spaces
    $_POST['name'] = trim($_POST['name']);

    # length > 4 and < 12
    if( (mb_strlen($_POST['name']) < 2) || (mb_strlen($_POST['name']) > 24) )       { $errors[] = _('Name must be between 4 and 24 characters'); }

10.2.111. Dependant Trait

10.2.111.1. Zencart

Dependant Trait, in app/library/zencart/CheckoutFlow/src/AccountFormValidator.php:14.

Note that addressEntries is used, and is also expected to be an array or an object with ArrayAccess. $addressEntries is only defined in a class called ‘Guest’ which is also the only one using that trait. Any other class using the AccountFormValidator trait must define addressEntries.

trait AccountFormValidator
{

    abstract protected function getAddressFieldValue($fieldName);

    /**
     * @return bool|int
     */
    protected function errorProcessing()
    {
        $error = false;
        foreach ($this->addressEntries as $fieldName => $fieldDetails) {
            $this->addressEntries[$fieldName]['value'] = $this->getAddressFieldValue($fieldName);
            $fieldError = $this->processFieldValidator($fieldName, $fieldDetails);
            $this->addressEntries[$fieldName]['error'] = $fieldError;
            $error = $error | $fieldError;
        }
        return $error;
    }

10.2.112. Hidden Use Expression

10.2.112.1. Tikiwiki

Hidden Use Expression, in lib/core/Tiki/Command/DailyReportSendCommand.php:17.

Sneaky error_reporting, hidden among the use calls.

namespace Tiki\Command;

use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
error_reporting(E_ALL);
use TikiLib;
use Reports_Factory;

10.2.112.2. OpenEMR

Hidden Use Expression, in interface/patient_file/summary/browse.php:23.

Use expression is only reached when the csrf token is checked. This probably save some CPU when no csrf is available, but it breaks the readability of the file.

<?php
/**
 * Patient selector for insurance gui
 *
 * @package   OpenEMR
 * @link      http://www.open-emr.org
 * @author    Brady Miller <brady.g.miller@gmail.com>
 * @copyright Copyright (c) 2018 Brady Miller <brady.g.miller@gmail.com>
 * @license   https://github.com/openemr/openemr/blob/master/LICENSE GNU General Public License 3
 */


require_once(../../globals.php);
require_once($srcdir/patient.inc);
require_once($srcdir/options.inc.php);

if (!empty($_POST)) {
    if (!verifyCsrfToken($_POST[csrf_token_form])) {
        csrfNotVerified();
    }
}

use OpenEMR\Core\Header;

10.2.113. Multiple Alias Definitions

10.2.113.1. ChurchCRM

Multiple Alias Definitions, in Various files:–.

It is actually surprising to find FamilyQuery defined as ChurchCRMBaseFamilyQuery only once, while all other reference are for ChurchCRMFamilyQuery. That lone use is actually useful in the code, so it is not a forgotten refactorisation.

use ChurchCRM\Base\FamilyQuery      // in /src/MapUsingGoogle.php:7

use ChurchCRM\FamilyQuery   // in /src/ChurchCRM/Dashboard/EventsDashboardItem.php:8
                            // and 29 other files

10.2.113.2. Phinx

Multiple Alias Definitions, in Various files:–.

One ‘Command’ is refering to a local Command class, while the other is refering to an imported class. They are all in a similar name space ConsoleCommand.

use Phinx\Console\Command                       //in file /src/Phinx/Console/PhinxApplication.php:34
use Symfony\Component\Console\Command\Command       //in file /src/Phinx/Console/Command/Init.php:31
use Symfony\Component\Console\Command\Command       //in file /src/Phinx/Console/Command/AbstractCommand.php:32

10.2.114. Cast To Boolean

10.2.114.1. MediaWiki

Cast To Boolean, in includes/page/WikiPage.php:2274.

$options[‘changed’] and $options[‘created’] are documented and used as boolean. Yet, SiteStatsUpdate may require integers, for correct storage in the database, hence the type casting. (int) (bool) may be an alternative here.

$edits = $options['changed'] ? 1 : 0;
            $pages = $options['created'] ? 1 : 0;


            DeferredUpdates::addUpdate( SiteStatsUpdate::factory(
                    [ 'edits' => $edits, 'articles' => $good, 'pages' => $pages ]
            ) );

10.2.114.2. Dolibarr

Cast To Boolean, in htdocs/societe/class/societe.class.php:2777.

Several cases are built on the same pattern there. Each of the expression may be replaced by a cast to (bool).

case 3:
                            $ret=(!$conf->global->SOCIETE_IDPROF3_UNIQUE?false:true);
                            break;

10.2.115. Failed Substr Comparison

10.2.115.1. Zurmo

Failed Substr Comparison, in app/protected/modules/zurmo/modules/SecurableModule.php:117.

filterAuditEvent compares a six char string with ‘AUDIT_EVENT_’ which contains 10 chars. This method returns only FALSE. Although it is used only once, the whole block that calls this method is now dead code.

private static function filterAuditEvent($s)
        {
            return substr($s, 0, 6) == 'AUDIT_EVENT_';
        }

10.2.115.2. MediaWiki

Failed Substr Comparison, in includes/media/DjVu.php:263.

$metadata contains data that may be in different formats. When it is a pure XML file, it is ‘Old style’. The comment helps understanding that this is not the modern way to go : the Old Style is actually never called, due to a failing condition.

private function getUnserializedMetadata( File $file ) {
            $metadata = $file->getMetadata();
            if ( substr( $metadata, 0, 3 ) === '<?xml' ) {
                    // Old style. Not serialized but instead just a raw string of XML.
                    return $metadata;
            }

10.2.116. Use Positive Condition

10.2.116.1. SPIP

Use Positive Condition, in ecrire/inc/utils.php:925.

if (isset($time[$t])) { } else { } would put the important case in first place, and be more readable.

if (!isset($time[$t])) {
            $time[$t] = $a + $b;
    } else {
            $p = ($a + $b - $time[$t]) * 1000;
            unset($time[$t]);
#                   echo "'$p'";exit;
            if ($raw) {
                    return $p;
            }
            if ($p < 1000) {
                    $s = '';
            } else {
                    $s = sprintf("%d ", $x = floor($p / 1000));
                    $p -= ($x * 1000);
            }

            return $s . sprintf($s ? "%07.3f ms" : "%.3f ms", $p);
    }

10.2.116.2. ExpressionEngine

Use Positive Condition, in system/ee/EllisLab/Addons/forum/mod.forum_core.php:9138.

Let’s be positive, and start processing the presence of $topic first. And let’s call it empty(), not == ‘’.

if ($topic != '')
                                            {
                                                    $sql .= '('.substr($topic, 0, -3).') OR ';
                                                    $sql .= '('.substr($tbody, 0, -3).') ';
                                            }
                                            else
                                            {
                                                    $sql = substr($sql, 0, -3);
                                            }

10.2.117. Don’t Echo Error

10.2.117.1. ChurchCRM

Don’t Echo Error, in wp-admin/includes/misc.php:74.

This is classic debugging code that should never reach production. mysqli_error() and mysqli_errno() provide valuable information is case of an error, and may be exploited by intruders.

if (mysqli_error($cnInfoCentral) != '') {
        echo gettext('An error occured: ').mysqli_errno($cnInfoCentral).'--'.mysqli_error($cnInfoCentral);
    } else {

10.2.117.2. Phpdocumentor

Don’t Echo Error, in src/phpDocumentor/Plugin/Graphs/Writer/Graph.php:77.

Default development behavior : display the caught exception. Production behavior should not display that message, but log it for later review. Also, the return in the catch should be moved to the main code sequence.

public function processClass(ProjectDescriptor $project, Transformation $transformation)
    {
        try {
            $this->checkIfGraphVizIsInstalled();
        } catch (\Exception $e) {
            echo $e->getMessage();

            return;
        }

10.2.118. No isset() With empty()

10.2.118.1. XOOPS

No isset() With empty(), in htdocs/class/tree.php:297.

Too much vlaidation

isset($this->tree[$key]['child']) && !empty($this->tree[$key]['child']);

10.2.119. Useless Check

10.2.119.1. Magento

Useless Check, in wp-admin/includes/misc.php:74.

This code assumes that $delete is an array, then checks if it empty. Foreach will take care of the empty check.

if (!empty($delete)) {
            foreach ($delete as $categoryId) {
                $where = array(
                    'product_id = ?'  => (int)$object->getId(),
                    'category_id = ?' => (int)$categoryId,
                );

                $write->delete($this->_productCategoryTable, $where);
            }
        }

10.2.119.2. Phinx

Useless Check, in src/Phinx/Migration/Manager.php:828.

If $dependencies is not empty, foreach() skips the loops.

private function getSeedDependenciesInstances(AbstractSeed $seed)
    {
        $dependenciesInstances = [];
        $dependencies = $seed->getDependencies();
        if (!empty($dependencies)) {
            foreach ($dependencies as $dependency) {
                foreach ($this->seeds as $seed) {
                    if (get_class($seed) === $dependency) {
                        $dependenciesInstances[get_class($seed)] = $seed;
                    }
                }
            }
        }

        return $dependenciesInstances;
    }

10.2.120. Bail Out Early

10.2.120.1. OpenEMR

Bail Out Early, in interface/modules/zend_modules/module/Carecoordination/src/Carecoordination/Controller/EncounterccdadispatchController.php:69.

This is a typical example of a function mostly controlled by one condition. It could be rewrite as ‘if($validResult != ‘existingpatient’)’ then return. The ‘else’ clause is not used anymore, and the whole block of code is now the main sequence of the method.

public function ccdaFetching($parameterArray = array())
    {
        $validResult = $this->getEncounterccdadispatchTable()->valid($parameterArray[0]);
        // validate credentials
        if ($validResult == 'existingpatient') {
/// Long bloc of code
        } else {
            return '<?xml version=1.0 encoding=UTF-8?>
                    <!-- Edited by XMLSpy -->
                    <note>

                            <heading>Authetication Failure</heading>
                            <body></body>
                    </note>
                    ';
        }

10.2.120.2. opencfp

Bail Out Early, in chair/assign_auto_reviewers_weighted_topic_match.inc:105.

This long example illustrates two aspects : first, the shortcut to the end of the method may be the ‘then’ clause, not necessarily the ‘else’. ‘!in_array($pid.’-‘.$rid, $conflictAR)’ leads to return, and the ‘else’ should be removed, while keeping its content. Secondly, we can see 3 conditions that all lead to a premature end to the method. After refactoring all of them, the method would end up with 1 level of indentation, instead of 3.

function oc_inConflict(&$conflictAR, $pid, $rid=null) {
    if ($rid == null) {
            $rid = $_SESSION[OCC_SESSION_VAR_NAME]['acreviewerid'];
    }
    if (!in_array($pid.'-'.$rid, $conflictAR)) {
            return false; // not in conflict
    } else {
            $tempr = ocsql_query("SELECT COUNT(*) AS `count` FROM `" . OCC_TABLE_PAPERREVIEWER . "` WHERE `paperid`='" . safeSQLstr($pid) . "' AND `reviewerid`='" . safeSQLstr($rid) . "'");
            if ((ocsql_num_rows($tempr) == 1)
                    && ($templ = ocsql_fetch_assoc($tempr))
                    && ($templ['count'] == 1)
            ) {
                    return false; // assigned as reviewer
            } else {
                    $tempr = ocsql_query("SELECT COUNT(*) AS `count` FROM `" . OCC_TABLE_PAPERADVOCATE . "` WHERE `paperid`='" . safeSQLstr($pid) . "' AND `advocateid`='" . safeSQLstr($rid) . "'");
                    if ((ocsql_num_rows($tempr) == 1)
                            && ($templ = ocsql_fetch_assoc($tempr))
                            && ($templ['count'] == 1)
                    ) {
                            return false; // assigned as advocate
                    }
            }
    }
    return true;
}

10.2.121. Too Many Local Variables

10.2.121.1. HuMo-Gen

Too Many Local Variables, in relations.php:813.

15 local variables pieces of code are hard to find in a compact form. This function shows one classic trait of such issue : a large ifthen is at the core of the function, and each time, it collects some values and build a larger string. This should probably be split between different methods in a class.

function calculate_nephews($generX) { // handed generations x is removed from common ancestor
global $db_functions, $reltext, $sexe, $sexe2, $language, $spantext, $selected_language, $foundX_nr, $rel_arrayX, $rel_arrayspouseX, $spouse;
global $reltext_nor, $reltext_nor2; // for Norwegian and Danish

    if($selected_language=="es"){
            if($sexe=="m") { $neph=__('nephew'); $span_postfix="o "; $grson='nieto'; }
            else { $neph=__('niece'); $span_postfix="a "; $grson='nieta'; }
            //$gendiff = abs($generX - $generY); // FOUT
            $gendiff = abs($generX - $generY) - 1;
            $gennr=$gendiff-1;
            $degree=$grson." ".$gennr.$span_postfix;
            if($gendiff ==1) { $reltext=$neph.__(' of ');}
            elseif($gendiff > 1 AND $gendiff < 27) {
                    spanish_degrees($gendiff,$grson);
                    $reltext=$neph." ".$spantext.__(' of ');
            }
            else { $reltext=$neph." ".$degree; }
    } elseif ($selected_language==he){
            if($sexe=='m') { $nephniece = __('nephew'); }
///............

10.2.122. Illegal Name For Method

10.2.122.1. PrestaShop

Illegal Name For Method, in admin-dev/ajaxfilemanager/inc/class.pagination.php:200.

__getBaseUrl and __setBaseUrl shouldn’t be named like that.

/**
     * get base url for pagination links aftr excluded those key
     * identified on excluded query strings
     *
     */
    function __getBaseUrl()
    {

            if(empty($this->baseUrl))
            {

                    $this->__setBaseUrl();
            }
            return $this->baseUrl;
    }

10.2.122.2. Magento

Illegal Name For Method, in app/code/core/Mage/Core/Block/Abstract.php:1139.

public method, called ‘__’. Example : $this->__();

public function __()
    {
        $args = func_get_args();
        $expr = new Mage_Core_Model_Translate_Expr(array_shift($args), $this->getModuleName());
        array_unshift($args, $expr);
        return $this->_getApp()->getTranslator()->translate($args);
    }

10.2.123. Long Arguments

10.2.123.1. Cleverstyle

Long Arguments, in core/drivers/DB/MySQLi.php:40.

This query is not complex, but its length tend to push the end out of the view in the IDE. It could be rewritten as a variable, on the previous line, with some formatting. The same formatting would help without the variable too, yet, mixing the SQL syntax with the PHP methodcall adds a layer of confusion.

$this->instance->query("SET SESSION sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'")

10.2.123.2. Contao

Long Arguments, in core-bundle/src/Resources/contao/widgets/CheckBoxWizard.php:145.

This one-liner includes 9 members and 6 variables : some are formatted by sprintf, some are directly concatenated in the string. Breaking this into two lines improves readbility and code review.

sprintf('<span><input type="checkbox" name="%s" id="opt_%s" class="tl_checkbox" value="%s"%s%s onfocus="Backend.getScrollOffset()"> %s<label for="opt_%s">%s</label></span>', $this->strName . ($this->multiple ? '[]' : ''), $this->strId . '_' . $i, ($this->multiple ? \StringUtil::specialchars($arrOption['value']) : 1), (((\is_array($this->varValue) && \in_array($arrOption['value'], $this->varValue)) || $this->varValue == $arrOption['value']) ? ' checked="checked"' : ''), $this->getAttributes( ), $strButtons, $this->strId . '_' . $i, $arrOption['label'])

10.2.124. No Boolean As Default

10.2.124.1. OpenConf

No Boolean As Default, in openconf/include.php:1264.

Why do we need a chair when printing a cell’s file ?

function oc_printFileCells(&$sub, $chair = false) { /**/ }

10.2.125. __DIR__ Then Slash

10.2.125.1. Traq

__DIR__ Then Slash, in src/Kernel.php:60.

When executed in a path ‘/a/b/c’, this code will require ‘/a../../vendor/autoload.php.

static::$loader = require __DIR__.'../../vendor/autoload.php';

10.2.126. Strange Name For Variables

10.2.126.1. FuelCMS

Strange Name For Variables, in fuel/modules/fuel/libraries/parser/dwoo/Dwoo/Adapters/CakePHP/dwoo.php:86.

Three _ is quite a lot for variables. Would they not be parameters but global variables, that would still be quite a lot.

public function _render($___viewFn, $___data_for_view, $___play_safe = true, $loadHelpers = true) {
    /**/
}

10.2.126.2. PhpIPAM

Strange Name For Variables, in app/admin/sections/edit-result.php:56.

$sss is the end-result of a progression, from $subsections (3s) to $ss to $sss. Although it is understandable from the code, a fuller name, like $subsection_subnet or $one_subsection_subnet would make this more readable.

//fetch subsection subnets
            foreach($subsections as $ss) {
                    $subsection_subnets = $Subnets->fetch_section_subnets($ss->id); //fetch all subnets in subsection
                    if(sizeof($subsection_subnets)>0) {
                            foreach($subsection_subnets as $sss) {
                                    $out[] = $sss;
                            }
                    }
                    $num_subnets = $num_subnets + sizeof($subsection_subnets);
                    //count all addresses that will be deleted!
                    $ipcnt = $Addresses->count_addresses_in_multiple_subnets($out);
            }

10.2.127. Check All Types

10.2.127.1. Zend-Config

Check All Types, in src/Writer/Ini.php:122.

$value must be an array or a string here.

foreach ($config as $key => $value) {
            $group = array_merge($parents, [$key]);

            if (is_array($value)) {
                $iniString .= $this->addBranch($value, $group);
            } else {
                $iniString .= implode($this->nestSeparator, $group)
                           .  ' = '
                           .  $this->prepareValue($value)
                           .  \n;
            }
        }

10.2.127.2. Vanilla

Check All Types, in src/Writer/Ini.php:122.

When $this->_FormValues is not null, then it is an array or an object, as it may be used immediately with foreach(). A check with is_array() would be a stronger option here.

if (is_null($this->_FormValues)) {
            $this->formValues();
        }

        $result = [[]];
        foreach ($this->_FormValues as $key => $value) {
            if (is_array($value)) {

10.2.128. Missing Cases In Switch

10.2.128.1. Tikiwiki

Missing Cases In Switch, in lib/articles/artlib.php:1075.

This switch handles 3 cases, plus the default for all others. There are other switch structures which also handle the ‘’ case. There may be a missing case here. In particular, projects/tikiwiki/code//article_image.php host another switch with the same case, plus another ‘topic’ case.

switch ($image_type) {
                    case 'article':
                            $image_cache_prefix = 'article';
                            break;
                    case 'submission':
                            $image_cache_prefix = 'article_submission';
                            break;
                    case 'preview':
                            $image_cache_prefix = 'article_preview';
                            break;
                    default:
                            return false;
            }

10.2.129. Repeated Regex

10.2.129.1. Vanilla

Repeated Regex, in library/core/class.pluginmanager.php:1200.

This regex is actually repeated 4 times across the Vanilla database, including this variation : ‘#^(https?:)?//#i’.

'`^https?://`'

10.2.129.2. Tikiwiki

Repeated Regex, in tiki-login.php:369.

This regex is use twice, identically, in the same file, with a few line of distance. It may be federated at the file level.

preg_match('/(tiki-register|tiki-login_validate|tiki-login_scr)\.php/', $url)

10.2.130. No Class In Global

10.2.130.1. Dolphin

No Class In Global, in Dolphin-v.7.3.5/inc/classes/BxDolXml.php:10.

This class should be put away in a ‘dolphin’ or ‘boonex’ namespace.

class BxDolXml {
    /* class BxDolXML code */
}

10.2.131. Could Use str_repeat()

10.2.131.1. Zencart

Could Use str_repeat(), in includes/functions/functions_general.php:1234.

That’s a 45 repeat of &nbsp;

if ( (!zen_browser_detect('MSIE')) && (zen_browser_detect('Mozilla/4')) ) {
      for ($i=0; $i<45; $i++) $pre .= '&nbsp;';
    }

10.2.132. Suspicious Comparison

10.2.132.1. PhpIPAM

Suspicious Comparison, in app/tools/vrf/index.php:110.

if $subnet[‘description’] is a string, the comparison with 0 turn it into a boolean. false’s length is 0, and true length is 1. PHP saves the day.

$subnet['description'] = strlen($subnet['description']==0) ? "/" : $subnet['description'];

10.2.132.2. ExpressionEngine

Suspicious Comparison, in ExpressionEngine_Core2.9.2/system/expressionengine/libraries/simplepie/SimplePie/Misc.php:1925.

If trim($attribs[‘’][‘mode’]) === ‘base64’, then it is set to lowercase (although it is already), and added to the && logical test. If it is ‘BASE64’, this fails.

if (isset($attribs['']['mode']) && strtolower(trim($attribs['']['mode']) === 'base64'))

10.2.133. Strings With Strange Space

10.2.133.1. OpenEMR

Strings With Strange Space, in library/globals.inc.php:3270.

The name of the contry contains both an unsecable space (the first, after Tonga), and a normal space (between Tonga and Islands). Translations are stored in a database, which preserves the unbreakable spaces. This also means that fixing the translation must be applied to every piece of data at the same time. The xl() function, which handles the translations, is also a good place to clean the spaces before searching for the right translation.

'to' => xl('Tonga (Tonga Islands)'),

10.2.133.2. Thelia

Strings With Strange Space, in templates/backOffice/default/I18n/fr_FR.php:647.

This is another example with a translation sentence. Here, the unbreakable space is before the question mark : this is a typography rule, that is common to many language. This would be a false positive, unless typography is handled by another part of the software.

'Mot de passe oublié ?'

10.2.134. No Empty Regex

10.2.134.1. Tikiwiki

No Empty Regex, in lib/sheet/excel/writer/worksheet.php:1925.

The initial ‘s’ seems to be too much. May be a typo ?

// Strip URL type
        $url = preg_replace('s[^internal:]', '', $url);

10.2.135. Randomly Sorted Arrays

10.2.135.1. Contao

Randomly Sorted Arrays, in system/modules/core/dca/tl_module.php:259.

The array array(‘maxlength’, ‘decodeEntities’, ‘tl_class’) is configured multiple times in this file. Most of them is in the second form, but some are in the first form. (Multiple occurrences in this file).

array('maxlength' => 255, 'decodeEntities' => true, 'tl_class' => 'w50') // Line 246
array('decodeEntities' => true, 'maxlength' => 255, 'tl_class' => 'w50'); // ligne 378

10.2.135.2. Vanilla

Randomly Sorted Arrays, in applications/dashboard/models/class.activitymodel.php:308.

‘Photo’ moved from last to second. This array is used with a ‘Join’ key, and is the base for a SQL table JOIN. As such, order is important. If this is the case, it seems unusual that the order is not the same for a join using the same tables. If it is not the case, arrays may be reordered.

/* L 305 */        Gdn::userModel()->joinUsers(
            $result->resultArray(),
            ['ActivityUserID', 'RegardingUserID'],
            ['Join' => ['Name', 'Email', 'Gender', 'Photo']]
        );

// L 385
        Gdn::userModel()->joinUsers($result, ['ActivityUserID', 'RegardingUserID'], ['Join' => ['Name', 'Photo', 'Email', 'Gender']]);

10.2.136. Only Variable Passed By Reference

10.2.136.1. Dolphin

Only Variable Passed By Reference, in administration/charts.json.php:89.

This is not possible, as array_slice() returns a new array, and not a reference. Minimally, the intermediate result must be saved in a variable, then popped. Actually, this code extracts the element at key 1 in the $aData array, although this also works with hash (non-numeric keys).

array_pop(array_slice($aData, 0, 1))

10.2.136.2. PhpIPAM

Only Variable Passed By Reference, in functions/classes/class.Thread.php:243.

This is sneaky bug : the assignation $status = 0 returns a value, and not a variable. This leads PHP to mistake the initialized 0 with the variable $status and fails. It is not possible to initialize variable AND use them as argument.

pcntl_waitpid($this->pid, $status = 0)

10.2.137. No Class As Typehint

10.2.137.1. Vanilla

No Class As Typehint, in library/Vanilla/Formatting/Formats/RichFormat.php:51.

All three typehints are based on classes. When Parser or Renderer are changed, for testing, versioning or moduling reasons, they must subclass the original class.

public function __construct(Quill\Parser $parser, Quill\Renderer $renderer, Quill\Filterer $filterer) {
        $this->parser = $parser;
        $this->renderer = $renderer;
        $this->filterer = $filterer;
    }

10.2.137.2. phpMyAdmin

No Class As Typehint, in libraries/classes/CreateAddField.php:29.

Although the class is named ‘DatabaseInterface’, it is a class.

public function __construct(DatabaseInterface $dbi)
    {
        $this->dbi = $dbi;
    }

10.2.138. No Return Used

10.2.138.1. SPIP

No Return Used, in ecrire/inc/utils.php:1067.

job_queue_remove() is called as an administration order, and the result is not checked. It is considered as a fire-and-forget command.

function job_queue_remove($id_job) {
    include_spip('inc/queue');

    return queue_remove_job($id_job);
}

10.2.138.2. LiveZilla

No Return Used, in livezilla/_lib/trdp/Zend/Loader.php:114.

The loadFile method tries to load a file, aka as include. If the inclusion fails, a PHP error is emitted (an exception would do the same), and there is not error management. Hence, the ‘return true;’, which is not tested later. It may be dropped.

public static function loadFile($filename, $dirs = null, $once = false)
    {
// A lot of code to check and include files

        return true;
    }

10.2.139. Mixed Concat And Interpolation

10.2.139.1. SuiteCrm

Mixed Concat And Interpolation, in modules/AOW_Actions/actions/actionSendEmail.php:89.

How long did it take to spot the hidden $checked variable in this long concatenation ? Using a consistent method of interpolation would help readability here.

"<input type='checkbox' id='aow_actions_param[" . $line . "][individual_email]' name='aow_actions_param[" . $line . "][individual_email]' value='1' $checked></td>"

10.2.139.2. Edusoho

Mixed Concat And Interpolation, in src/AppBundle/Controller/Admin/SiteSettingController.php:168.

Calling a method from a property of an object is possible inside a string, though it is rare. Setting the method outside the string make it more readable.

"{$this->container->getParameter('topxia.upload.public_url_path')}/" . $parsed['path']

10.2.140. Too Many Injections

10.2.140.1. NextCloud

Too Many Injections, in lib/private/Share20/Manager.php:130.

Well documented Manager class. Quite a lot of injections though, it must take a long time to prepare it.

/**
     * Manager constructor.
     *
     * @param ILogger $logger
     * @param IConfig $config
     * @param ISecureRandom $secureRandom
     * @param IHasher $hasher
     * @param IMountManager $mountManager
     * @param IGroupManager $groupManager
     * @param IL10N $l
     * @param IFactory $l10nFactory
     * @param IProviderFactory $factory
     * @param IUserManager $userManager
     * @param IRootFolder $rootFolder
     * @param EventDispatcher $eventDispatcher
     * @param IMailer $mailer
     * @param IURLGenerator $urlGenerator
     * @param \OC_Defaults $defaults
     */
    public function __construct(
                    ILogger $logger,
                    IConfig $config,
                    ISecureRandom $secureRandom,
                    IHasher $hasher,
                    IMountManager $mountManager,
                    IGroupManager $groupManager,
                    IL10N $l,
                    IFactory $l10nFactory,
                    IProviderFactory $factory,
                    IUserManager $userManager,
                    IRootFolder $rootFolder,
                    EventDispatcher $eventDispatcher,
                    IMailer $mailer,
                    IURLGenerator $urlGenerator,
                    \OC_Defaults $defaults
    ) {
            $this->logger = $logger;
            $this->config = $config;
            $this->secureRandom = $secureRandom;
            $this->hasher = $hasher;
            $this->mountManager = $mountManager;
            $this->groupManager = $groupManager;
            $this->l = $l;
            $this->l10nFactory = $l10nFactory;
            $this->factory = $factory;
            $this->userManager = $userManager;
            $this->rootFolder = $rootFolder;
            $this->eventDispatcher = $eventDispatcher;
            $this->sharingDisabledForUsersCache = new CappedMemoryCache();
            $this->legacyHooks = new LegacyHooks($this->eventDispatcher);
            $this->mailer = $mailer;
            $this->urlGenerator = $urlGenerator;
            $this->defaults = $defaults;
    }

10.2.140.2. Thelia

Too Many Injections, in lib/private/Share20/Manager.php:130.

Classic address class, with every details. May be even shorter than expected.

//class DeliveryPostageEvent extends ActionEvent
    public function __construct(
        DeliveryModuleInterface $module,
        Cart $cart,
        Address $address = null,
        Country $country = null,
        State $state = null
    ) {
        $this->module = $module;
        $this->cart = $cart;
        $this->address = $address;
        $this->country = $country;
        $this->state = $state;
    }

10.2.141. @ Operator

10.2.141.1. Phinx

@ Operator, in src/Phinx/Util/Util.php:239.

fopen() may be tested for existence, readability before using it. Although, it actually emits some errors on Windows, with network volumes.

$isReadable = @\fopen($filePath, 'r') !== false;

        if (!$filePath || !$isReadable) {
            throw new \Exception(sprintf(Cannot open file %s \n, $filename));
        }

10.2.141.2. PhpIPAM

@ Operator, in functions/classes/class.Log.php:322.

Variable and index existence should always be tested with isset() : it is faster than using @.

$_SESSION['ipamusername']

10.2.142. Mismatched Ternary Alternatives

10.2.142.1. phpadsnew

Mismatched Ternary Alternatives, in phpAdsNew-2.0/admin/lib-misc-stats.inc.php:219.

This is an unusual way to apply a condition. $bgcolor is ‘#FFFFFF’ by default, and if $i % 2, then $bcolor is ‘#F6F6F6’;. A more readable ternary option would be ‘$bgcolor = = $i % 2 ? “#FFFFFF” : “#F6F6F6”;’, and make a matched alternative branches.

$bgcolor = #FFFFFF;
    $i % 2 ? 0 : $bgcolor = #F6F6F6;

10.2.142.2. OpenEMR

Mismatched Ternary Alternatives, in portal/messaging/messages.php:132.

IS_DASHBOARD is defined as a boolean or a string. Later, it is tested as a boolean, and displayed as a integer, which will be cast to string by echo. Lots of transtyping are happening here.

// In two distinct if/then branch
l:29) define('IS_DASHBOARD', false);
l:41) define('IS_DASHBOARD', $_SESSION['authUser']);

l:132) echo IS_DASHBOARD ? IS_DASHBOARD : 0;
?>

10.2.143. Mismatched Default Arguments

10.2.143.1. SPIP

Mismatched Default Arguments, in ecrire/inc/lien.php:160.

generer_url_entite() takes $connect in, with a default value of empty string. Later, generer_url_entite() receives that value, but uses null as a default value. This forces the ternary test on $connect, to turn it into a null before shipping it to the next function, and having it processed accordingly.

// http://code.spip.net/@traiter_lien_implicite
function traiter_lien_implicite($ref, $texte = '', $pour = 'url', $connect = '') {

    // some code was edited here

    if (is_array($url)) {
            @list($type, $id) = $url;
            $url = generer_url_entite($id, $type, $args, $ancre, $connect ? $connect : null);
    }

10.2.144. Mismatched Typehint

10.2.144.1. WordPress

Mismatched Typehint, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.145. Assign With And

10.2.145.1. xataface

Assign With And, in Dataface/LanguageTool.php:265.

The usage of ‘and’ here is a workaround for PHP version that have no support for the coalesce. $autosubmit receives the value of $params[‘autosubmit’] only if the latter is set. Yet, with = having higher precedence over ‘and’, $autosubmit is mistaken with the existence of $params[‘autosubmit’] : its value is actually omitted.

$autosubmit = isset($params['autosubmit']) and $params['autosubmit'];

10.2.146. Logical To in_array

10.2.146.1. Zencart

Logical To in_array, in admin/users.php:32.

Long list of == are harder to read. Using an in_array() call gathers all the strings together, in an array. In turn, this helps readability and possibility, reusability by making that list an constant.

// if needed, check that a valid user id has been passed
if (($action == 'update' || $action == 'reset') && isset($_POST['user']))
{
  $user = $_POST['user'];
}
elseif (($action == 'edit' || $action == 'password' || $action == 'delete' || $action == 'delete_confirm') && $_GET['user'])
{
  $user = $_GET['user'];
}
elseif(($action=='delete' || $action=='delete_confirm') && isset($_POST['user']))
{
  $user = $_POST['user'];
}

10.2.147. Pathinfo() Returns May Vary

10.2.147.1. NextCloud

Pathinfo() Returns May Vary, in lib/private/Preview/Office.php:56.

$absPath is build with the toTmpFile() method, which may return a boolean (false) in case of error. Error situations include the inability to create the temporary file.

$absPath = $fileview->toTmpFile($path);

// More code

                    list($dirname, , , $filename) = array_values(pathinfo($absPath));
                    $pngPreview = $dirname . '/' . $filename . '.png';

10.2.148. Multiple Type Variable

10.2.148.1. Typo3

Multiple Type Variable, in typo3/sysext/backend/Classes/Form/Element/InputDateTimeElement.php:270.

$fullElement is an array most of the time, but finally ends up being a string. Since the array is not the final state, it may be interesting to make it a class, which collects the various variables, and export the final string. Such class would be usefull in several places in this repository.

$fullElement = [];
            $fullElement[] = '<div class=checkbox t3js-form-field-eval-null-placeholder-checkbox>';
            $fullElement[] =     '<label for= . $nullControlNameEscaped . >';
            $fullElement[] =         '<input type=hidden name= . $nullControlNameEscaped .  value= . $fallbackValue .  />';
            $fullElement[] =         '<input type=checkbox name= . $nullControlNameEscaped .  id= . $nullControlNameEscaped .  value=1' . $checked . $disabled . ' />';
            $fullElement[] =         $overrideLabel;
            $fullElement[] =     '</label>';
            $fullElement[] = '</div>';
            $fullElement[] = '<div class=t3js-formengine-placeholder-placeholder>';
            $fullElement[] =    '<div class=form-control-wrap style=max-width: . $width . px>';
            $fullElement[] =        '<input type=text class=form-control disabled=disabled value= . $shortenedPlaceholder .  />';
            $fullElement[] =    '</div>';
            $fullElement[] = '</div>';
            $fullElement[] = '<div class=t3js-formengine-placeholder-formfield>';
            $fullElement[] =    $expansionHtml;
            $fullElement[] = '</div>';
            $fullElement = implode(LF, $fullElement);

10.2.148.2. Vanilla

Multiple Type Variable, in typo3/sysext/backend/Classes/Form/Element/InputDateTimeElement.php:270.

Here, $value may be of different type. The if() structures merges all the incoming format into one standard type (int). This is actually the contrary of this analysis, and is a false positive.

if (is_array($value)) {
                        $value = count($value);
                    } elseif (stringEndsWith($field, 'UserID', true)) {
                        $value = 1;
                    }


vanilla     $value = count($value)  /library/core/functions.general.php:1427

10.2.149. Is Actually Zero

10.2.149.1. Dolibarr

Is Actually Zero, in htdocs/compta/ajaxpayment.php:99.

Here, the $amountToBreakDown is either $currentRemain or $result.

$amountToBreakdown = ($result - $currentRemain >= 0 ?
                                                                            $currentRemain :                                                                // Remain can be fully paid
                                                                            $currentRemain + ($result - $currentRemain));   // Remain can only partially be paid

10.2.149.2. SuiteCrm

Is Actually Zero, in modules/AOR_Charts/lib/pChart/class/pDraw.class.php:523.

$Xa may only amount to $iX2, though the expression looks weird.

if ( $X > $iX2 ) { $Xa = $X-($X-$iX2); $Ya = $iY1+($X-$iX2); } else { $Xa = $X; $Ya = $iY1; }

10.2.150. Could Be Else

10.2.150.1. SugarCrm

Could Be Else, in SugarCE-Full-6.5.26/modules/Emails/ListViewGroup.php:79.

The first condition makes different checks if ‘query’ is in $_REQUEST or not. The second only applies to $_REQUEST[‘query’], as there is no else. There is also no visible sign that the first condition may change $_REQUEST or not

if(!isset($_REQUEST['query'])){
    //_pp('loading: '.$currentModule.'Group');
    //_pp($current_user->user_preferences[$currentModule.'GroupQ']);
    $storeQuery->loadQuery($currentModule.'Group');
    $storeQuery->populateRequest();
} else {
    //_pp($current_user->user_preferences[$currentModule.'GroupQ']);
    //_pp('saving: '.$currentModule.'Group');
    $storeQuery->saveFromGet($currentModule.'Group');
}

if(isset($_REQUEST['query'])) {
    // we have a query
    if(isset($_REQUEST['email_type']))                              $email_type = $_REQUEST['email_type'];
    if(isset($_REQUEST['assigned_to']))                             $assigned_to = $_REQUEST['assigned_to'];
    if(isset($_REQUEST['status']))                                  $status = $_REQUEST['status'];
    // More code
}

10.2.150.2. OpenEMR

Could Be Else, in library/log.inc:653.

Those two if structure may definitely merged into one single instruction.

$success = 1;
    $checksum = ;
    if ($outcome === false) {
        $success = 0;
    }

    if ($outcome !== false) {
        // Should use the $statement rather than the processed
        // variables, which includes the binded stuff. If do
        // indeed need the binded values, then will need
        // to include this as a separate array.

        //error_log(STATEMENT: .$statement,0);
        //error_log(BINDS: .$processed_binds,0);
        $checksum = sql_checksum_of_modified_row($statement);
        //error_log(CHECKSUM: .$checksum,0);
    }

10.2.151. Next Month Trap

10.2.151.1. Contao

Next Month Trap, in system/modules/calendar/classes/Events.php:515.

This code is wrong on August 29,th 30th and 31rst : 6 months before is caculated here as February 31rst, so march 2. Of course, this depends on the leap years.

case 'past_180':
                            return array(strtotime('-6 months'), time(), $GLOBALS['TL_LANG']['MSC']['cal_empty']);

10.2.151.2. Edusoho

Next Month Trap, in src/AppBundle/Controller/Admin/AnalysisController.php:1426.

The last month is wrong 8 times a year : on 31rst, and by the end of March.

'lastMonthStart' => date('Y-m-d', strtotime(date('Y-m', strtotime('-1 month')))),
            'lastMonthEnd' => date('Y-m-d', strtotime(date('Y-m', time())) - 24 * 3600),
            'lastThreeMonthsStart' => date('Y-m-d', strtotime(date('Y-m', strtotime('-2 month')))),

10.2.152. Don’t Send $this In Constructor

10.2.152.1. Woocommerce

Don’t Send $this In Constructor, in includes/class-wc-cart.php:107.

WC_Cart_Session and WC_Cart_Fees receives $this, the current object, at a moment where it is not consistent : for example, tax_display_cart hasn’t been set yet. Although it may be unexpected to have an object called WC_Cart being called by the session or the fees, this is still a temporary inconsistence.

/**
     * Constructor for the cart class. Loads options and hooks in the init method.
     */
    public function __construct() {
            $this->session          = new WC_Cart_Session( $this );
            $this->fees_api         = new WC_Cart_Fees( $this );
            $this->tax_display_cart = $this->is_tax_displayed();

            // Register hooks for the objects.
            $this->session->init();

10.2.152.2. Contao

Don’t Send $this In Constructor, in system/modules/core/library/Contao/Model.php:110.

$this is send to $objRegistry. $objRegistry is obtained with a factory, ModelRegistry::getInstance(). It is probably fully prepared at that point. Yet, $objRegistry is called and used to fill $this properties with full values. At some point, $objRegistry return values without having a handle on a fully designed object.

/**
     * Load the relations and optionally process a result set
     *
     * @param \Database\Result $objResult An optional database result
     */
    public function __construct(\Database\Result $objResult=null)
    {
        // Some code was removed
                    $objRegistry = \Model\Registry::getInstance();

                    $this->setRow($arrData); // see #5439
                    $objRegistry->register($this);

        // More code below
        // $this-> are set
        // $objRegistry is called
    }

10.2.153. Parent First

10.2.153.1. shopware

Parent First, in wp-admin/includes/misc.php:74.

Here, the parent is called last. Givent that $title is defined in the same class, it seems that $name may be defined in the BaseContainer class. In fact, it is not, and BasecContainer and FieldSet are fairly independant classes. Thus, the parent::__construct call could be first here, though more as a coding convention.

/**
 * Class FieldSet
 */
class FieldSet extends BaseContainer
{
    /**
     * @var string
     */
    protected $title;

    /**
     * @param string $name
     * @param string $title
     */
    public function __construct($name, $title)
    {
        $this->title = $title;
        $this->name = $name;
        parent::__construct();
    }

10.2.153.2. PrestaShop

Parent First, in wp-admin/includes/misc.php:74.

A good number of properties are set in the current object even before the parent AdminController(Core) is called. ‘table’ and ‘lang’ acts as default values for the parent class, as it (the parent class) would set them to another default value. Many properties are used, but not defined in the current class, nor its parent. This approach prevents the constructor from requesting too many arguments. Yet, as such, it is difficult to follow which of the initial values are transmitted via protected/public properties rather than using the __construct() call.

class AdminWebserviceControllerCore extends AdminController
{
    /** this will be filled later */
    public $fields_form = array('webservice form');
    protected $toolbar_scroll = false;

    public function __construct()
    {
        $this->bootstrap = true;
        $this->table = 'webservice_account';
        $this->className = 'WebserviceKey';
        $this->lang = false;
        $this->edit = true;
        $this->delete = true;
        $this->id_lang_default = Configuration::get('PS_LANG_DEFAULT');

        parent::__construct();

10.2.154. Invalid Regex

10.2.154.1. SugarCrm

Invalid Regex, in SugarCE-Full-6.5.26/include/utils/file_utils.php:513.

This yields an error at execution time : ``Compilation failed: invalid range in character class at offset 4 ``.

preg_replace('/[^\w-._]+/i', '', $name)

10.2.155. Identical On Both Sides

10.2.155.1. phpMyAdmin

Identical On Both Sides, in libraries/classes/DatabaseInterface.php:323.

This code looks like ($options & DatabaseInterface::QUERY_STORE) == DatabaseInterface::QUERY_STORE, which would make sense. But PHP precedence is actually executing $options & (DatabaseInterface::QUERY_STORE == DatabaseInterface::QUERY_STORE), which then doesn’t depends on QUERY_STORE but only on $options.

if ($options & DatabaseInterface::QUERY_STORE == DatabaseInterface::QUERY_STORE) {
    $tmp = $this->_extension->realQuery('
        SHOW COUNT(*) WARNINGS', $this->_links[$link], DatabaseInterface::QUERY_STORE
    );
    $warnings = $this->fetchRow($tmp);
} else {
    $warnings = 0;
}

10.2.155.2. HuMo-Gen

Identical On Both Sides, in libraries/classes/DatabaseInterface.php:323.

In that long logical expression, $personDb->pers_cal_date is tested twice

// *** Filter person's WITHOUT any date's ***
                    if ($user[group_filter_date]=='j'){
                            if ($personDb->pers_birth_date=='' AND $personDb->pers_bapt_date==''
                            AND $personDb->pers_death_date=='' AND $personDb->pers_buried_date==''
                            AND $personDb->pers_cal_date=='' AND $personDb->pers_cal_date==''
                            ){
                                    $privacy_person='';
                            }
                    }

10.2.156. No Reference For Ternary

10.2.156.1. phpadsnew

No Reference For Ternary, in lib/OA/Admin/Menu/Section.php334:334.

The reference should be removed from the function definition. Either this method returns null, which is never a reference, or it returns $this, which is always a reference, or the results of a methodcall. The latter may or may not be a reference, but the Ternary operator will drop it and return by value.

function &getParentOrSelf($type)
    {
        if ($this->type == $type) {
            return $this;
        }
        else {
            return $this->parentSection != null ? $this->parentSection->getParentOrSelf($type) : null;
        }
    }

10.2.157. Unused Inherited Variable In Closure

10.2.157.1. shopware

Unused Inherited Variable In Closure, in recovery/update/src/app.php:129.

In the first closuree, $containere is used as the root for the method calls, but $app is not used. It may be dropped. In fact, some of the following calls to $app->map() only request one inherited, $container.

$app->map('/applyMigrations', function () use ($app, $container) {
    $container->get('controller.batch')->applyMigrations();
})->via('GET', 'POST')->name('applyMigrations');

$app->map('/importSnippets', function () use ($container) {
    $container->get('controller.batch')->importSnippets();
})->via('GET', 'POST')->name('importSnippets');

10.2.157.2. Mautic

Unused Inherited Variable In Closure, in MauticCrmBundle/Tests/Integration/SalesforceIntegrationTest.php:1202.

$max is relayed to getLeadsToCreate(), while $restart is omitted. It may be dropped, along with its reference.

function () use (&$restart, $max) {
                    $args = func_get_args();

                    if (false === $args[2]) {
                        return $max;
                    }

                    $createLeads = $this->getLeadsToCreate($args[2], $max);

                    // determine whether to return a count or records
                    if (false === $args[2]) {
                        return count($createLeads);
                    }

                    return $createLeads;
                }

10.2.158. Useless Referenced Argument

10.2.158.1. Woocommerce

Useless Referenced Argument, in includes/data-stores/class-wc-product-variation-data-store-cpt.php:414.

$product is defined with a reference in the method signature, but it is also used as an object with a dynamical property. As such, the reference in the argument definition is too much.

public function update_post_meta( &$product, $force = false ) {
            $meta_key_to_props = array(
                    '_variation_description' => 'description',
            );

            $props_to_update = $force ? $meta_key_to_props : $this->get_props_to_update( $product, $meta_key_to_props );

            foreach ( $props_to_update as $meta_key => $prop ) {
                                    $value   = $product->{get_$prop}( 'edit' );
                                    $updated = update_post_meta( $product->get_id(), $meta_key, $value );
                    if ( $updated ) {
                            $this->updated_props[] = $prop;
                    }
            }

            parent::update_post_meta( $product, $force );

10.2.158.2. Magento

Useless Referenced Argument, in setup/src/Magento/Setup/Module/Di/Compiler/Config/Chain/PreferencesResolving.php:63.

$value is defined with a reference. In the following code, it is only read and never written : for index search, or by itself. In fact, $preferences is also only read, and never written. As such, both could be removed.

private function resolvePreferenceRecursive(&$value, &$preferences)
    {
        return isset($preferences[$value])
            ? $this->resolvePreferenceRecursive($preferences[$value], $preferences)
            : $value;
    }

10.2.159. Test Then Cast

10.2.159.1. Dolphin

Test Then Cast, in wp-admin/includes/misc.php:74.

$aLimits[‘per_page’] is tested for existence and not false. Later, it is cast from string to int : yet, a ‘0.1’ string value would pass the test, and end up filling $aLimits[‘per_page’] with 0.

if (isset($aLimits['per_page']) && $aLimits['per_page'] !== false)
            $this->aCurrent['paginate']['perPage'] = (int)$aLimits['per_page'];

10.2.159.2. SuiteCrm

Test Then Cast, in modules/jjwg_Maps/controller.php:1035.

$marker[‘lat’] is compared to the string ‘0’, which actually transtype it to integer, then it is cast to string for map_marker_data_points() needs and finally, it is cast to float, in case of a correction. It would be safer to test it in its string type, since floats are not used as array indices.

if ($marker['lat'] != '0' && $marker['lng'] != '0') {

            // Check to see if marker point already exists and apply offset if needed
            // This often occurs when an address is only defined by city, state, zip.
            $i = 0;
            while (isset($this->map_marker_data_points[(string) $marker['lat']][(string) $marker['lng']]) &&
            $i < $this->settings['map_markers_limit']) {
                $marker['lat'] = (float) $marker['lat'] + (float) $this->settings['map_duplicate_marker_adjustment'];
                $marker['lng'] = (float) $marker['lng'] + (float) $this->settings['map_duplicate_marker_adjustment'];
                $i++;
            }

10.2.160. Redefined Private Property

10.2.160.1. Zurmo

Redefined Private Property, in app/protected/modules/zurmo/models/OwnedCustomField.php:51.

The class OwnedCustomField is part of a large class tree : OwnedCustomField extends CustomField, CustomField extends BaseCustomField, BaseCustomField extends RedBeanModel, RedBeanModel extends BeanModel.

Since $canHaveBean is distinct in BeanModel and in OwnedCustomField, the public method getCanHaveBean() also had to be overloaded.

class OwnedCustomField extends CustomField
    {
        /**
         * OwnedCustomField does not need to have a bean because it stores no attributes and has no relations
         * @see RedBeanModel::canHaveBean();
         * @var boolean
         */
        private static $canHaveBean = false;

/..../

        /**
         * @see RedBeanModel::getHasBean()
         */
        public static function getCanHaveBean()
        {
            if (get_called_class() == 'OwnedCustomField')
            {
                return self::$canHaveBean;
            }
            return parent::getCanHaveBean();
        }

10.2.161. Don’t Unset Properties

10.2.161.1. Vanilla

Don’t Unset Properties, in applications/dashboard/models/class.activitymodel.php:1073.

The _NotificationQueue property, in this class, is defined as an array. Here, it is destroyed, then recreated. The unset() is too much, as the assignation is sufficient to reset the array

/**
     * Clear notification queue.
     *
     * @since 2.0.17
     * @access public
     */
    public function clearNotificationQueue() {
        unset($this->_NotificationQueue);
        $this->_NotificationQueue = [];
    }

10.2.161.2. Typo3

Don’t Unset Properties, in typo3/sysext/linkvalidator/Classes/Linktype/InternalLinktype.php:73.

The property errorParams is emptied by unsetting it. The property is actually defined in the above class, as an array. Until the next error is added to this list, any access to the error list has to be checked with isset(), or yield an ‘Undefined’ warning.

public function checkLink($url, $softRefEntry, $reference)
    {
        $anchor = '';
        $this->responseContent = true;
        // Might already contain values - empty it
        unset($this->errorParams);
//....

abstract class AbstractLinktype implements LinktypeInterface
{
    /**
     * Contains parameters needed for the rendering of the error message
     *
     * @var array
     */
    protected $errorParams = [];

10.2.162. Strtr Arguments

10.2.162.1. SuiteCrm

Strtr Arguments, in includes/vCard.php:221.

This code prepares incoming ‘$values’ for extraction. The keys are cleaned then split with explode(). The ‘=’ sign would stay, as strtr() can’t remove it. This means that such keys won’t be recognized later in the code, and gets omitted.

$values = explode(';', $value);
                    $key = strtoupper($keyvalue[0]);
                    $key = strtr($key, '=', '');
                    $key = strtr($key, ',', ';');
                    $keys = explode(';', $key);

10.2.163. Callback Needs Return

10.2.163.1. Contao

Callback Needs Return, in core-bundle/src/Resources/contao/modules/ModuleQuicklink.php:91.

The empty closure returns null. The array_flip() array has now all its values set to null, and reset, as intended. A better alternative is to use the array_fill_keys() function, which set a default value to every element of an array, once provided with the expected keys.

$arrPages = array_map(function () {}, array_flip($tmp));

10.2.163.2. Phpdocumentor

Callback Needs Return, in core-bundle/src/Resources/contao/modules/ModuleQuicklink.php:91.

The array_walk() function is called on the plugin’s list. Each element is registered with the application, but is not used directly : this is for later. The error mechanism is to throw an exception : this is the only expected feedback. As such, no return is expected. May be a ‘foreach’ loop would be more appropriate here, but this is syntactic sugar.

array_walk(
            $plugins,
            function ($plugin) use ($app) {
                /** @var Plugin $plugin */
                $provider = (strpos($plugin->getClassName(), '\') === false)
                    ? sprintf('phpDocumentor\Plugin\%s\ServiceProvider', $plugin->getClassName())
                    : $plugin->getClassName();
                if (!class_exists($provider)) {
                    throw new \RuntimeException('Loading Service Provider for ' . $provider . ' failed.');
                }

                try {
                    $app->register(new $provider($plugin));
                } catch (\InvalidArgumentException $e) {
                    throw new \RuntimeException($e->getMessage());
                }
            }
        );

10.2.164. Wrong Range Check

10.2.164.1. Dolibarr

Wrong Range Check, in htdocs/includes/phpoffice/PhpSpreadsheet/Spreadsheet.php:1484.

When $tabRatio is 1001, then the condition is valid, and the ratio accepted. The right part of the condition is not executed.

public function setTabRatio($tabRatio)
    {
        if ($tabRatio >= 0 || $tabRatio <= 1000) {
            $this->tabRatio = (int) $tabRatio;
        } else {
            throw new Exception('Tab ratio must be between 0 and 1000.');
        }
    }

10.2.164.2. WordPress

Wrong Range Check, in wp-includes/formatting.php:3634.

This condition may be easier to read as $diff >= WEEK_IN_SECONDS && $diff < MONTH_IN_SECONDS. When testing for outside this interval, using not is also more readable : !($diff >= WEEK_IN_SECONDS && $diff < MONTH_IN_SECONDS).

} elseif ( $diff < MONTH_IN_SECONDS && $diff >= WEEK_IN_SECONDS ) {
            $weeks = round( $diff / WEEK_IN_SECONDS );
            if ( $weeks <= 1 ) {
                    $weeks = 1;
            }
            /* translators: Time difference between two dates, in weeks. %s: Number of weeks */
            $since = sprintf( _n( '%s week', '%s weeks', $weeks ), $weeks );

10.2.165. Cant Instantiate Class

10.2.165.1. WordPress

Cant Instantiate Class, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.166. strpos() Too Much

10.2.166.1. WordPress

strpos() Too Much, in core/traits/Request/Server.php:127.

Instead of searching for HTTP_, it is faster to compare the first 5 chars to the literal HTTP_. In case of absence, this solution returns faster.

if (strpos($header, 'HTTP_') === 0) {
                            $header = substr($header, 5);
                    } elseif (strpos($header, 'CONTENT_') !== 0) {
                            continue;
                    }

10.2.167. Weak Typing

10.2.167.1. TeamPass

Weak Typing, in includes/libraries/Tree/NestedTree/NestedTree.php:100.

The is_null() test detects a special situation, that requires usage of default values. The ‘else’ handles every other situations, including when the $node is an object, or anything else. $this->getNode() will gain from having typehints : it may be NULL, or the results of mysqli_fetch_object() : a stdClass object. The expected properties of nleft and nright are not certain to be available.

public function getDescendants($id = 0, $includeSelf = false, $childrenOnly = false, $unique_id_list = false)
    {
        global $link;
        $idField = $this->fields['id'];

        $node = $this->getNode($id);
        if (is_null($node)) {
            $nleft = 0;
            $nright = 0;
            $parent_id = 0;
            $personal_folder = 0;
        } else {
            $nleft = $node->nleft;
            $nright = $node->nright;
            $parent_id = $node->$idField;
            $personal_folder = $node->personal_folder;
        }

10.2.168. Mismatch Type And Default

10.2.168.1. WordPress

Mismatch Type And Default, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.169. Bad Constants Names

10.2.169.1. PrestaShop

Bad Constants Names, in src/PrestaShopBundle/Install/Upgrade.php:214.

INSTALL_PATH is a valid name for a constant. __PS_BASE_URI__ is not a valid name.

require_once(INSTALL_PATH . 'install_version.php');
            // needed for upgrade before 1.5
            if (!defined('__PS_BASE_URI__')) {
                define('__PS_BASE_URI__', str_replace('//', '/', '/'.trim(preg_replace('#/(install(-dev)?/upgrade)$#', '/', str_replace('\', '/', dirname($_SERVER['REQUEST_URI']))), '/').'/'));
            }

10.2.169.2. Zencart

Bad Constants Names, in zc_install/ajaxTestDBConnection.php:10.

A case where PHP needs help : if the PHP version is older than 5.3, then it is valid to compensate. Though, this __DIR__ has a fixed value, wherever it is used, while the official __DIR__ change from dir to dir.

if (!defined('__DIR__')) define('__DIR__', dirname(__FILE__));

10.2.170. Abstract Or Implements

10.2.170.1. Zurmo

Abstract Or Implements, in app/protected/extensions/zurmoinc/framework/views/MassEditProgressView.php:30.

The class MassEditProgressView extends ProgressView, which is an abstract class. That class defines one abstract method : abstract protected function headerLabelPrefixContent(). Yet, the class MassEditProgressView doesn’t implements this method. This means that the class can’t be instatiated, and indeed, it isn’t. The class MassEditProgressView is subclassed, by the class MarketingListMembersMassSubscribeProgressView, which implements the method headerLabelPrefixContent(). As such, MassEditProgressView should be marked abstract, so as to prevent any instantiation attempt.

class MassEditProgressView extends ProgressView {
    /**/
}

10.2.171. Incompatible Signature Methods

10.2.171.1. SuiteCrm

Incompatible Signature Methods, in modules/Home/Dashlets/RSSDashlet/RSSDashlet.php:138.

The class in the RSSDashlet.php file has an ‘array’ typehint which is not in the parent Dashlet class. While both files compile separately, they yield a PHP warning when running : typehinting mismatch only yields a warning.

// File /modules/Home/Dashlets/RSSDashlet/RSSDashlet.php
    public function saveOptions(
        array $req
        )
    {

// File /include/Dashlets/Dashlets.php
    public function saveOptions( $req ) {

10.2.172. Ambiguous Visibilities

10.2.172.1. Typo3

Ambiguous Visibilities, in typo3/sysext/backend/Classes/Controller/NewRecordController.php:90.

$allowedNewTables is declared once protected and once public. $allowedNewTables is rare : 2 occurences. This may lead to confusion about access to this property.

class NewRecordController
{
/.. many lines../
    /**
     * @var array
     */
    protected $allowedNewTables;

class DatabaseRecordList
{
/..../
    /**
     * Used to indicate which tables (values in the array) that can have a
     * create-new-record link. If the array is empty, all tables are allowed.
     *
     * @var string[]
     */
    public $allowedNewTables = [];

10.2.173. Could Be Abstract Class

10.2.173.1. Edusoho

Could Be Abstract Class, in src/Biz/Task/Strategy/BaseStrategy.php:14.

BaseStrategy is extended by NormalStrategy, DefaultStrategy (Not shown here), but it is not instantiated itself.

class BaseStrategy {
    // Class code
}

10.2.173.2. shopware

Could Be Abstract Class, in engine/Shopware/Plugins/Default/Core/PaymentMethods/Components/GenericPaymentMethod.php:31.

A ‘Generic’ class sounds like a class that could be ‘abstract’.

class GenericPaymentMethod extends BasePaymentMethod {
    // More class code
}

10.2.174. Method Could Be Static

10.2.174.1. FuelCMS

Method Could Be Static, in fuel/modules/fuel/models/Fuel_assets_model.php:240.

This method makes no usage of $this : it only works on the incoming argument, $file. This may even be a function.

public function get_file($file)
    {
            // if no extension is provided, then we determine that it needs to be decoded
            if (strpos($file, '.') === FALSE)
            {
                    $file = uri_safe_decode($file);
            }
            return $file;
    }

10.2.174.2. ExpressionEngine

Method Could Be Static, in system/ee/legacy/libraries/Upload.ph:859.

This method returns the list of mime type, by using a hidden global value : ee() is a functioncall that give access to the external storage of values.

/**
     * List of Mime Types
     *
     * This is a list of mime types.  We use it to validate
     * the allowed types set by the developer
     *
     * @param       string
     * @return      string
     */
    public function mimes_types($mime)
    {
            ee()->load->library('mime_type');
            return ee()->mime_type->isSafeForUpload($mime);
    }

10.2.175. Possible Missing Subpattern

10.2.175.1. phpMyAdmin

Possible Missing Subpattern, in libraries/classes/Advisor.php:557.

The last capturing subpattern is ( \[(.*)\])? and it is optional. Indeed, when the pattern succeed, the captured values are stored in $match. Yet, the code checks for the existence of $match[3] before using it.

if (preg_match("/rule\s'(.*)'( \[(.*)\])?$/", $line, $match)) {
                    $ruleLine = 1;
                    $ruleNo++;
                    $rules[$ruleNo] = ['name' => $match[1]];
                    $lines[$ruleNo] = ['name' => $i + 1];
                    if (isset($match[3])) {
                        $rules[$ruleNo]['precondition'] = $match[3];
                        $lines[$ruleNo]['precondition'] = $i + 1;
                    }

10.2.175.2. SPIP

Possible Missing Subpattern, in ecrire/inc/filtres_dates.php:73.

This code avoid the PHP notice by padding the resulting array (see comment in French : eviter === avoid)

if (preg_match("#^([12][0-9]{3}[-/][01]?[0-9])([-/]00)?( [-0-9:]+)?$#", $date, $regs)) {
                            $regs = array_pad($regs, 4, null); // eviter notice php
                            $date = preg_replace("@/@", "-", $regs[1]) . "-00" . $regs[3];
                    } else {
                            $date = date("Y-m-d H:i:s", strtotime($date));
                    }

10.2.176. Could Be Private Class Constant

10.2.176.1. Phinx

Could Be Private Class Constant, in src/Phinx/Db/Adapter/MysqlAdapter.php:46.

The code includes a fair number of class constants. The one listed here are only used to define TEXT columns in MySQL, with their maximal size. Since they are only intented to be used by the MySQL driver, they may be private.

class MysqlAdapter extends PdoAdapter implements AdapterInterface
{

//.....
    const TEXT_SMALL   = 255;
    const TEXT_REGULAR = 65535;
    const TEXT_MEDIUM  = 16777215;
    const TEXT_LONG    = 4294967295;

10.2.177. One Letter Functions

10.2.177.1. ThinkPHP

One Letter Functions, in ThinkPHP/Mode/Api/functions.php:859.

There are also the functions C, E, G… The applications is written in a foreign language, which may be a base for non-significant function names.

function F($name, $value = '', $path = DATA_PATH)

10.2.177.2. Cleverstyle

One Letter Functions, in core/drivers/DB/PostgreSQL.php:71.

There is also function f(). Those are actually overwritten methods. From the documentation, q() is for query, and f() is for fetch. Those are short names for frequently used functions.

public function q ($query, ...$params) {

10.2.178. __debugInfo() Usage

10.2.178.1. Dolibarr

__debugInfo() Usage, in htdocs/includes/stripe/lib/StripeObject.php:108.

_values is a private property from the Stripe Class. The class contains other objects, but only _values are displayed with var_dump.

// Magic method for var_dump output. Only works with PHP >= 5.6
    public function __debugInfo()
    {
        return $this->_values;
    }

10.2.179. PHP7 Dirname

10.2.179.1. OpenConf

PHP7 Dirname, in include.php:61.

Since PHP 7.0, dirname( , 2); does the job.

$OC_basepath = dirname(dirname($_SERVER['PHP_SELF']));

10.2.179.2. MediaWiki

PHP7 Dirname, in includes/installer/Installer.php:1173.

Since PHP 7.0, dirname( , 2); does the job.

protected function envPrepPath() {
            global $IP;
            $IP = dirname( dirname( __DIR__ ) );
            $this->setVar( 'IP', $IP );
    }

10.2.180. Avoid set_error_handler $context Argument

10.2.180.1. shopware

Avoid set_error_handler $context Argument, in engine/Shopware/Plugins/Default/Core/ErrorHandler/Bootstrap.php:162.

The registered handler is a local method, called errorHandler, which has 6 arguments, and relays those 6 arguments to set_error_handler().

public function registerErrorHandler($errorLevel = E_ALL)
    {
        // Only register once.  Avoids loop issues if it gets registered twice.
        if (self::$_registeredErrorHandler) {
            set_error_handler([$this, 'errorHandler'], $errorLevel);

            return $this;
        }

        self::$_origErrorHandler = set_error_handler([$this, 'errorHandler'], $errorLevel);
        self::$_registeredErrorHandler = true;

        return $this;
    }

10.2.180.2. Vanilla

Avoid set_error_handler $context Argument, in library/core/functions.error.php:747.

Gdn_ErrorHandler is a function that requires 6 arguments.

set_error_handler('Gdn_ErrorHandler', E_ALL & ~E_STRICT)

10.2.181. Unused Private Properties

10.2.181.1. OpenEMR

Unused Private Properties, in entities/User.php:46.

This class has a long list of private properties. It also has an equally long (minus one) list of accessors, and a __toString() method which exposes all of them. $oNotes is the only one never mentionned anywhere.

class User
{
    /**
     * @Column(name=id, type=integer)
     * @GeneratedValue(strategy=AUTO)
     */
    private $id;

    /**
     * @OneToMany(targetEntity=ONote, mappedBy=user)
     */
    private $oNotes;

10.2.181.2. phpadsnew

Unused Private Properties, in lib/OA/Admin/UI/component/Form.php:23.

$dispatcher is never used anywhere.

class OA_Admin_UI_Component_Form
    extends HTML_QuickForm
{
    private $dispatcher;

10.2.182. Unused Functions

10.2.182.1. Woocommerce

Unused Functions, in includes/wc-core-functions.php:2124.

wc_is_external_resource() is unused. This is not obvious immediately, since there is a call from wc_get_relative_url(). Yet since wc_get_relative_url() itself is never used, then it is a dead function. As such, since wc_is_external_resource() is only called by this first function, it also dies, even though it is called in the code.

/**
 * Make a URL relative, if possible.
 *
 * @since 3.2.0
 * @param string $url URL to make relative.
 * @return string
 */
function wc_get_relative_url( $url ) {
    return wc_is_external_resource( $url ) ? $url : str_replace( array( 'http://', 'https://' ), '//', $url );
}

/**
 * See if a resource is remote.
 *
 * @since 3.2.0
 * @param string $url URL to check.
 * @return bool
 */
function wc_is_external_resource( $url ) {
    $wp_base = str_replace( array( 'http://', 'https://' ), '//', get_home_url( null, '/', 'http' ) );

    return strstr( $url, '://' ) && ! strstr( $url, $wp_base );
}

10.2.182.2. Piwigo

Unused Functions, in admin/include/functions.php:2167.

get_user_access_level_html_options() is unused and can’t be find in the code.

/**
 * Returns access levels as array used on template with html_options functions.
 *
 * @param int $MinLevelAccess
 * @param int $MaxLevelAccess
 * @return array
 */
function get_user_access_level_html_options($MinLevelAccess = ACCESS_FREE, $MaxLevelAccess = ACCESS_CLOSED)
{
  $tpl_options = array();
  for ($level = $MinLevelAccess; $level <= $MaxLevelAccess; $level++)
  {
    $tpl_options[$level] = l10n(sprintf('ACCESS_%d', $level));
  }
  return $tpl_options;
}

10.2.183. Exception Order

10.2.183.1. Woocommerce

Exception Order, in includes/api/v1/class-wc-rest-products-controller.php:787.

This try/catch expression is able to catch both WC_Data_Exception and WC_REST_Exception.

In another file, /includes/api/class-wc-rest-exception.php, we find that WC_REST_Exception extends WC_Data_Exception (class WC_REST_Exception extends WC_Data_Exception {}). So WC_Data_Exception is more general, and a WC_REST_Exception exception is caught with WC_Data_Exception Exception. The second catch should be put in first.

This code actually loads the file, join it, then split it again. file() would be sufficient.

try {
                    $product_id = $this->save_product( $request );
                    $post       = get_post( $product_id );
                    $this->update_additional_fields_for_object( $post, $request );
                    $this->update_post_meta_fields( $post, $request );

                    /**
                     * Fires after a single item is created or updated via the REST API.
                     *
                     * @param WP_Post         $post      Post data.
                     * @param WP_REST_Request $request   Request object.
                     * @param boolean         $creating  True when creating item, false when updating.
                     */
                    do_action( 'woocommerce_rest_insert_product', $post, $request, false );
                    $request->set_param( 'context', 'edit' );
                    $response = $this->prepare_item_for_response( $post, $request );

                    return rest_ensure_response( $response );
            } catch ( WC_Data_Exception $e ) {
                    return new WP_Error( $e->getErrorCode(), $e->getMessage(), $e->getErrorData() );
            } catch ( WC_REST_Exception $e ) {
                    return new WP_Error( $e->getErrorCode(), $e->getMessage(), array( 'status' => $e->getCode() ) );
            }

10.2.184. Rethrown Exceptions

10.2.184.1. PrestaShop

Rethrown Exceptions, in classes/webservice/WebserviceOutputBuilder.php:731.

The setSpecificField method catches a WebserviceException, representing an issue with the call to the webservice. However, that piece of information is lost, and the exception is rethrown immediately, without any action.

public function setSpecificField($object, $method, $field_name, $entity_name)
    {
            try {
                    $this->validateObjectAndMethod($object, $method);
            } catch (WebserviceException $e) {
                    throw $e;
            }

            $this->specificFields[$field_name] = array('entity'=>$entity_name, 'object' => $object, 'method' => $method, 'type' => gettype($object));
            return $this;
    }

10.2.185. Slow Functions

10.2.185.1. ChurchCRM

Slow Functions, in src/Reports/PrintDeposit.php:35.

You may replace this with a isset() : $_POST can’t contain a NULL value, unless it was set by the script itself.

array_key_exists("report_type", $_POST);

10.2.185.2. SuiteCrm

Slow Functions, in include/json_config.php:242.

This is a equivalent for nl2br()

preg_replace("/\r\n/", "<BR>", $focus->$field)

10.2.186. Joining file()

10.2.186.1. WordPress

Joining file(), in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.186.2. SPIP

Joining file(), in ecrire/inc/install.php:109.

When the file is not accessible, file() returns null, and can’t be processed by join().

$s = @join('', file($file));

10.2.186.3. ExpressionEngine

Joining file(), in ExpressionEngine_Core2.9.2/system/expressionengine/libraries/simplepie/idn/idna_convert.class.php:100.

join(‘’, ) is used as a replacement for file_get_contents(), which was introduced in PHP 4.3.0.

if (function_exists('file_get_contents')) {
    $this->NP = unserialize(file_get_contents(dirname(__FILE__).'/npdata.ser'));
} else {
    $this->NP = unserialize(join('', file(dirname(__FILE__).'/npdata.ser')));
}

10.2.186.4. PrestaShop

Joining file(), in classes/module/Module.php:2972.

implode(‘’, ) is probably not the slowest part in these lines.

$override_file = file($override_path);

eval(preg_replace(array('#^\s*<\?(?:php)?#', '#class\s+'.$classname.'\s+extends\s+([a-z0-9_]+)(\s+implements\s+([a-z0-9_]+))?#i'), array(' ', 'class '.$classname.'OverrideOriginal_remove'.$uniq), implode('', $override_file)));
$override_class = new ReflectionClass($classname.'OverrideOriginal_remove'.$uniq);

$module_file = file($this->getLocalPath().'override/'.$path);
eval(preg_replace(array('#^\s*<\?(?:php)?#', '#class\s+'.$classname.'(\s+extends\s+([a-z0-9_]+)(\s+implements\s+([a-z0-9_]+))?)?#i'), array(' ', 'class '.$classname.'Override_remove'.$uniq), implode('', $module_file)));

10.2.187. Make One Call With Array

10.2.187.1. HuMo-Gen

Make One Call With Array, in admin/include/kcfinder/lib/helper_text.php:47.

The three calls to str_replace() could be replaced by one, using array arguments. Nesting the calls doesn’t reduce the number of calls.

static function jsValue($string) {
        return
            preg_replace('/\r?\n/', "\n",
            str_replace('"', "\\"",
            str_replace("'", "\'",
            str_replace("\", "\\",
        $string))));
    }

10.2.187.2. Edusoho

Make One Call With Array, in src/AppBundle/Common/StringToolkit.php:55.

Since str_replace is already using an array, the second argument must also be an array, with repeated empty strings. That syntax allows adding the ‘&nbsp;’ and ‘ ‘ to those arrays. Note also that trim() should be be called early, but since some of the replacing may generate terminal spaces, it should be kept as is.

$text = strip_tags($text);

        $text = str_replace(array(\n, \r, \t), '', $text);
        $text = str_replace('&nbsp;', ' ', $text);
        $text = trim($text);

10.2.188. No Count With 0

10.2.188.1. Contao

No Count With 0, in system/modules/repository/classes/RepositoryManager.php:1148.

If $elist contains at least one element, then it is not empty().

$ext->found = count($elist)>0;

10.2.188.2. WordPress

No Count With 0, in wp-admin/includes/misc.php:74.

$build or $signature are empty at that point, no need to calculate their respective length.

// Check for zero length, although unlikely here
    if (strlen($built) == 0 || strlen($signature) == 0) {
      return false;
    }

10.2.189. time() Vs strtotime()

10.2.189.1. Woocommerce

time() Vs strtotime(), in includes/class-wc-webhook.php:384.

time() would be faster here, as an entropy generator. Yet, it would still be better to use an actual secure entropy generator, like random_byte or random_int. In case of older version, microtime() would yield better entropy.

public function get_new_delivery_id() {
            // Since we no longer use comments to store delivery logs, we generate a unique hash instead based on current time and webhook ID.
            return wp_hash( $this->get_id() . strtotime( 'now' ) );
    }

10.2.190. Avoid glob() Usage

10.2.190.1. Phinx

Avoid glob() Usage, in src/Phinx/Migration/Manager.php:362.

glob() searches for a list of files in the migration folder. Those files are not known, but they have a format, as checked later with the regex : a combinaison of FilesystemIterator and RegexIterator would do the trick too.

$phpFiles = glob($config->getMigrationPath() . DIRECTORY_SEPARATOR . '*.php');

            // filter the files to only get the ones that match our naming scheme
            $fileNames = array();
            /** @var AbstractMigration[] $versions */
            $versions = array();

            foreach ($phpFiles as $filePath) {
                if (preg_match('/([0-9]+)_([_a-z0-9]*).php/', basename($filePath))) {

10.2.190.2. NextCloud

Avoid glob() Usage, in lib/private/legacy/helper.php:185.

Recursive copy of folders, based on scandir(). DirectoryIterator and FilesystemIterator would do the same without the recursion.

static function copyr($src, $dest) {
            if (is_dir($src)) {
                    if (!is_dir($dest)) {
                            mkdir($dest);
                    }
                    $files = scandir($src);
                    foreach ($files as $file) {
                            if ($file != "." && $file != "..") {
                                    self::copyr("$src/$file", "$dest/$file");
                            }
                    }
            } elseif (file_exists($src) && !\OC\Files\Filesystem::isFileBlacklisted($src)) {
                    copy($src, $dest);
            }
    }

10.2.191. Avoid Concat In Loop

10.2.191.1. SuiteCrm

Avoid Concat In Loop, in include/export_utils.php:433.

$line is build in several steps, then then final version is added to $content. It would be much faster to make $content an array, and implode it once after the loop.

foreach($records as $record)
        {
            $line = implode("\"" . getDelimiter() . "\"", $record);
            $line = "\"" . $line;
            $line .= "\"\r\n";
            $line = parseRelateFields($line, $record, $customRelateFields);
            $content .= $line;
        }

10.2.191.2. ThinkPHP

Avoid Concat In Loop, in include/export_utils.php:433.

The foreach loop prepares the ‘getControllerRoute’ call, then, accumulates all the resulting strings in $content. It would be much faster to make $content an array, and implode it once after the loop.

foreach ($controllers as $controller) {
            $controller = basename($controller, '.php');

            $class = new \ReflectionClass($namespace . '\' . $module . '\' . $layer . '\' . $controller);

            if (strpos($layer, '\')) {
                // 多级控制器
                $level      = str_replace(DIRECTORY_SEPARATOR, '.', substr($layer, 11));
                $controller = $level . '.' . $controller;
                $length     = strlen(strstr($layer, '\', true));
            } else {
                $length = strlen($layer);
            }

            if ($suffix) {
                $controller = substr($controller, 0, -$length);
            }

            $content .= $this->getControllerRoute($class, $module, $controller);
        }

10.2.192. Use pathinfo() Arguments

10.2.192.1. Zend-Config

Use pathinfo() Arguments, in src/Factory.php:74:90.

The $filepath is broken into pieces, and then, only the ‘extension’ part is used. With the PATHINFO_EXTENSION constant used as a second argument, only this value could be returned.

$pathinfo = pathinfo($filepath);

        if (! isset($pathinfo['extension'])) {
            throw new Exception\RuntimeException(sprintf(
                'Filename "%s" is missing an extension and cannot be auto-detected',
                $filename
            ));
        }

        $extension = strtolower($pathinfo['extension']);
        // Only $extension is used beyond that point

10.2.192.2. ThinkPHP

Use pathinfo() Arguments, in ThinkPHP/Extend/Library/ORG/Net/UploadFile.class.php:508.

Without any other check, pathinfo() could be used with PATHINFO_EXTENSION.

private function getExt($filename) {
        $pathinfo = pathinfo($filename);
        return $pathinfo['extension'];
    }

10.2.193. Substring First

10.2.193.1. SPIP

Substring First, in ecrire/inc/filtres.php:1694.

The code first makes everything uppercase, including the leading and trailing spaces, and then, removes them : it would be best to swap those operations. Note that spip_substr() is not considered in this analysis, but with SPIP knowledge, it could be moved inside the calls.

function filtre_initiale($nom) {
    return spip_substr(trim(strtoupper(extraire_multi($nom))), 0, 1);
}

10.2.193.2. PrestaShop

Substring First, in admin-dev/filemanager/include/utils.php:197.

dirname() reduces the string (or at least, keeps it the same size), so it more efficient to have it first.

dirname(str_replace(' ', '~', $str))

10.2.194. Slice Arrays First

10.2.194.1. WordPress

Slice Arrays First, in modules/InboundEmail/InboundEmail.php:1080.

Instead of reading ALL the keys, and then, keeping only the first fifty, why not read the 50 first items from the array, and then extract the keys?

$results = array_slice(array_keys($diff), 0 ,50);

10.2.195. Double array_flip()

10.2.195.1. NextCloud

Double array_flip(), in lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php:372.

The array $allowedScriptDomains is flipped, to unset ‘self’, then, unflipped (or flipped again), to restore its initial state. Using array_keys() or array_search() would yield the needed keys for unsetting, at a lower cost.

if(is_string($this->useJsNonce)) {
                            $policy .= '\'nonce-'.base64_encode($this->useJsNonce).'\'';
                            $allowedScriptDomains = array_flip($this->allowedScriptDomains);
                            unset($allowedScriptDomains['\'self\'']);
                            $this->allowedScriptDomains = array_flip($allowedScriptDomains);
                            if(count($allowedScriptDomains) !== 0) {
                                    $policy .= ' ';
                            }
                    }

10.2.196. Closure Could Be A Callback

10.2.196.1. Tine20

Closure Could Be A Callback, in tine20/Tinebase/Convert/Json.php:318.

is_scalar() is sufficient here.

$value = array_filter($value, function ($val) { return is_scalar($val); });

10.2.196.2. NextCloud

Closure Could Be A Callback, in apps/files_sharing/lib/ShareBackend/Folder.php:114.

$qb is the object for the methodcall, passed via use. The closure may have been replaced with array($qb, ‘createNamedParameter’).

$parents = array_map(function($parent) use ($qb) {
                            return $qb->createNamedParameter($parent);
                    }, $parents);

10.2.197. Isset() On The Whole Array

10.2.197.1. Tine20

Isset() On The Whole Array, in tine20/Crm/Model/Lead.php:208.

Only the second call is necessary : it also includes the first one.

isset($relation['related_record']) && isset($relation['related_record']['n_fileas'])

10.2.197.2. ExpressionEngine

Isset() On The Whole Array, in system/ee/legacy/libraries/Form_validation.php:1487.

This is equivalent to isset($this->_field_data[$field], $this->_field_data[$field][‘postdata’]), and the second call may be skipped.

!isset($this->_field_data[$field]) OR !isset($this->_field_data[$field]['postdata'])

10.2.198. Compare Hash

10.2.198.1. Traq

Compare Hash, in src/Models/User.php:105.

This code should also avoid using SHA1.

sha1($password) == $this->password

10.2.198.2. LiveZilla

Compare Hash, in livezilla/_lib/objects.global.users.inc.php:1391.

This code is using the stronger SHA256 but compares it to another string. $_token may be non-empty, and still be comparable to 0.

function IsValidToken($_token)
{
    if(!empty($_token))
        if(hash("sha256",$this->Token) == $_token)
            return true;
    return false;
}

10.2.199. Register Globals

10.2.199.1. TeamPass

Register Globals, in api/index.php:25.

The API starts with security features, such as the whitelist(). The whitelist applies to IP addresses, so the query string is not sanitized. Then, the QUERY_STRING is parsed, and creates a lot of new global variables.

teampass_whitelist();

parse_str($_SERVER['QUERY_STRING']);
$method = $_SERVER['REQUEST_METHOD'];
$request = explode("/", substr(@$_SERVER['PATH_INFO'], 1));

10.2.199.2. XOOPS

Register Globals, in htdocs/modules/system/admin/images/main.php:33:33.

This code only exports the POST variables as globals. And it does clean incoming variables, but not all of them.

// Check users rights
if (!is_object($xoopsUser) || !is_object($xoopsModule) || !$xoopsUser->isAdmin($xoopsModule->mid())) {
    exit(_NOPERM);
}

//  Check is active
if (!xoops_getModuleOption('active_images', 'system')) {
    redirect_header('admin.php', 2, _AM_SYSTEM_NOTACTIVE);
}

if (isset($_POST)) {
    foreach ($_POST as $k => $v) {
        ${$k} = $v;
    }
}

// Get Action type
$op = system_CleanVars($_REQUEST, 'op', 'list', 'string');

10.2.200. Safe Curl Options

10.2.200.1. OpenConf

Safe Curl Options, in openconf/include.php:703.

The function that holds that code is only used to call openconf.com, over http, while openconf.com is hosted on https, nowadays. This may be a sign of hard to access certificates.

$ch = curl_init();
                    curl_setopt($ch, CURLOPT_URL, $f);
                    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
                    curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
                    curl_setopt($ch, CURLOPT_AUTOREFERER, true);
                    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);
                    curl_setopt($ch, CURLOPT_MAXREDIRS, 5);
                    curl_setopt($ch, CURLOPT_HEADER, false);
                    $s = curl_exec($ch);
                    curl_close($ch);
                    return($s);

10.2.201. Unserialize Second Arg

10.2.201.1. Piwigo

Unserialize Second Arg, in admin/configuration.php:491.

unserialize() extracts information from the $conf variable : this variable is read from a configuration file. It is later tested to be an array, whose index may not be all set (@$disabled[$type];). It would be safer to make $disabled an object, add the class to unserialize, and set default values to the needed properties/index.

$disabled = @unserialize(@$conf['disabled_derivatives']);

10.2.201.2. LiveZilla

Unserialize Second Arg, in livezilla/_lib/objects.global.inc.php:2600.

unserialize() only extract a non-empty value here. But its content is not checked. It is later used as an array, with multiple index.

$this->Customs = (!empty($_row["customs"])) ? @unserialize($_row["customs"]) : array();

10.2.202. Encoded Simple Letters

10.2.202.1. Zurmo

Encoded Simple Letters, in yii/framework/web/CClientScript.php:783.

This actually decodes into a copyright notice.

‘function cleanAndSanitizeScriptHeader(& $output)
{
$requiredOne = <span>Copyright &#169; Zurmo Inc., 2013. All rights reserved.;….’
eval(\x66\x75\x6e\x63\x74\x69\x6f\x6e\x20\x63\x6c\x65\x61\x6e\x41\x6e\x64\x53\x61\x6e\x69\x74\x69\x7a\x65\x53\x63\x72 .
     \x69\x70\x74\x48\x65\x61\x64\x65\x72\x28\x26\x20\x24\x6f\x75\x74\x70\x75\x74\x29\x0d\x0a\x20\x20\x20\x20\x20\x20 .
     \x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x7b\x0d\x0a\x20\x20\x20\x20\x20\x20\x20 .
     \x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x20\x24\x72\x65\x71\x75\x69\x72 .
     // several more lines like that

10.2.203. Mkdir Default

10.2.203.1. Mautic

Mkdir Default, in app/bundles/CoreBundle/Helper/AssetGenerationHelper.php:120.

This code is creating some directories for Javascript or CSS (from the directories names) : those require universal reading access, but probably no execution nor writing access. 0711 would be sufficient in this case.

//combine the files into their corresponding name and put in the root media folder
                if ($env == 'prod') {
                    $checkPaths = [
                        $assetsFullPath,
                        $assetsFullPath/css,
                        $assetsFullPath/js,
                    ];
                    array_walk($checkPaths, function ($path) {
                        if (!file_exists($path)) {
                            mkdir($path);
                        }
                    });

10.2.203.2. OpenEMR

Mkdir Default, in interface/main/backuplog.php:27.

If $BACKUP_EVENTLOG_DIR is a backup for an event log, this should be stored out of the web server reach, with low rights, beside the current user. This is part of a CLI PHP script.

mkdir($BACKUP_EVENTLOG_DIR)

10.2.204. Phpinfo

10.2.204.1. Dolphin

Phpinfo, in Dolphin-v.7.3.5/install/exec.php:4.

An actual phpinfo(), available during installation. Note that the phpinfo() is actually triggered by a hidden POST variable.

<?php

    if (!empty($_POST['phpinfo']))
        phpinfo();
    elseif (!empty($_POST['gdinfo']))
        echo '<pre>' . print_r(gd_info(), true) . '</pre>';

?>
<center>

    <form method=post>
        <input type=submit name=phpinfo value="PHP Info">
    </form>
    <form method=post>
        <input type=submit name=gdinfo value="GD Info">
    </form>

</center>

10.2.205. Argument Should Be Typehinted

10.2.205.1. Dolphin

Argument Should Be Typehinted, in Dolphin-v.7.3.5/plugins/intervention-image/Intervention/Image/Gd/Commands/WidenCommand.php:20.

This closures make immediate use of the $constraint argument, and calls its method aspectRatio. No check is made on this argument, and it may easily be mistaken with another class, or a null. Adding a typehint here will ensure a more verbose development error and help detect misuse of the closure.

$this->arguments[2] = function ($constraint) use ($additionalConstraints) {
            $constraint->aspectRatio();
            if(is_callable($additionalConstraints))
                $additionalConstraints($constraint);
        };

10.2.205.2. Mautic

Argument Should Be Typehinted, in app/bundles/PluginBundle/Helper/IntegrationHelper.php:374.

This piece of code inside a 275 lines method. Besides, there are 11 classes that offer a ‘getPriority’ method, although $returnServices could help to semantically reduce the number of possible classes. Here, typehints on $a and $b help using the wrong kind of object.

if (empty($alphabetical)) {
            // Sort by priority
            uasort($returnServices, function ($a, $b) {
                $aP = (int) $a->getPriority();
                $bP = (int) $b->getPriority();

                if ($aP === $bP) {
                    return 0;
                }

                return ($aP < $bP) ? -1 : 1;
            });

10.2.206. Could Return Void

10.2.206.1. WordPress

Could Return Void, in wp-admin/includes/misc.php:74.

In fact, RunQuery returns a boolean, which should be also returned here. No Void needed if the last called method is not returning void too.

function RemoveVolunteerOpportunity($iPersonID, $iVolID)
{
    $sSQL = 'DELETE FROM person2volunteeropp_p2vo WHERE p2vo_per_ID = '.$iPersonID.' AND p2vo_vol_ID = '.$iVolID;
    RunQuery($sSQL);
}

10.2.207. Isset Multiple Arguments

10.2.207.1. ThinkPHP

Isset Multiple Arguments, in library/think/Request.php:1187.

This may be shortened with isset($sub), $array[$name][$sub])

isset($sub) && isset($array[$name][$sub])

10.2.207.2. LiveZilla

Isset Multiple Arguments, in livezilla/_lib/trdp/pchart/class/pDraw.class.php:3852.

This is the equivalent of !(isset($Data[“Series”][$SerieA][“Data”]) && isset($Data[“Series”][$SerieB][“Data”])), and then, !(isset($Data[“Series”][$SerieA][“Data”], $Data[“Series”][$SerieB][“Data”]))

!isset($Data["Series"][$SerieA]["Data"]) || !isset($Data["Series"][$SerieB]["Data"])

10.2.208. Unitialized Properties

10.2.208.1. SPIP

Unitialized Properties, in ecrire/public/interfaces.php:584.

The class Critere (Criteria) has no method at all. When using a class as an array, to capture values, one of the advantage of the class is in the default values for the properties. In particular, the last property here, called $not, should be initialized with a false.

/**
 * Description d'un critère de boucle
 *
 * Sous-noeud de Boucle
 *
 * @package SPIP\Core\Compilateur\AST
 **/
class Critere {
    /**
     * Type de noeud
     *
     * @var string
     */
    public $type = 'critere';

    /**
     * Opérateur (>, <, >=, IN, ...)
     *
     * @var null|string
     */
    public $op;

    /**
     * Présence d'une négation (truc !op valeur)
     *
     * @var null|string
     */
    public $not;

10.2.209. Use List With Foreach

10.2.209.1. MediaWiki

Use List With Foreach, in includes/parser/LinkHolderArray.php:372.

This foreach reads each element from $entries into entry. $entry, in turn, is written into $pdbk, $title and $displayText for easier reuse. 5 elements are read from $entry, and they could be set in their respective variable in the foreach() with a list call. The only on that can’t be set is ‘query’ which has to be tested.

foreach ( $entries as $index => $entry ) {
                            $pdbk = $entry['pdbk'];
                            $title = $entry['title'];
                            $query = isset( $entry['query'] ) ? $entry['query'] : [];
                            $key = "$ns:$index";
                            $searchkey = "<!--LINK'\" $key-->";
                            $displayText = $entry['text'];
                            if ( isset( $entry['selflink'] ) ) {
                                    $replacePairs[$searchkey] = Linker::makeSelfLinkObj( $title, $displayText, $query );
                                    continue;
                            }
                            if ( $displayText === '' ) {
                                    $displayText = null;
                            } else {
                                    $displayText = new HtmlArmor( $displayText );
                            }
                            if ( !isset( $colours[$pdbk] ) ) {
                                    $colours[$pdbk] = 'new';
                            }
                            $attribs = [];
                            if ( $colours[$pdbk] == 'new' ) {
                                    $linkCache->addBadLinkObj( $title );
                                    $output->addLink( $title, 0 );
                                    $link = $linkRenderer->makeBrokenLink(
                                            $title, $displayText, $attribs, $query
                                    );
                            } else {
                                    $link = $linkRenderer->makePreloadedLink(
                                            $title, $displayText, $colours[$pdbk], $attribs, $query
                                    );
                            }

                            $replacePairs[$searchkey] = $link;
                    }

10.2.210. Empty With Expression

10.2.210.1. HuMo-Gen

Empty With Expression, in fanchart.php:297.

The test on $pid may be directly done on $treeid[$sosa][0]. The distance between the assignation and the empty() makes it hard to spot.

$pid=$treeid[$sosa][0];
                    $birthyr=$treeid[$sosa][1];
                    $deathyr=$treeid[$sosa][4];
                    $fontpx=$fontsize;
                    if($sosa>=16 AND $fandeg==180) { $fontpx=$fontsize-1; }
                    if($sosa>=32 AND $fandeg!=180) { $fontpx=$fontsize-1; }
                    if (!empty($pid)) {

10.2.211. Should Use array_filter()

10.2.211.1. xataface

Should Use array_filter(), in actions/manage_build_index.php:38.

This selection process has three tests : the two first are exclusive, and the third is inclusive. They could fit in one or several closures.

$indexable = array();
            foreach ( $tables as $key=>$table ){
                    if ( preg_match('/^dataface__/', $table) ){
                            continue;
                    }
                    if ( preg_match('/^_/', $table) ){
                            continue;
                    }

                    if ( $index->isTableIndexable($table) ){
                            $indexable[] = $table;
                            //unset($tables[$key]);
                    }

            }

10.2.211.2. shopware

Should Use array_filter(), in engine/Shopware/Bundle/StoreFrontBundle/Service/Core/VariantCoverService.php:71.

Closure would be the best here, since $covers has to be injected in the array_filter callback.

$covers = $this->variantMediaGateway->getCovers(
            $products,
            $context
        );

        $fallback = [];
        foreach ($products as $product) {
            if (!array_key_exists($product->getNumber(), $covers)) {
                $fallback[] = $product;
            }
        }

10.2.212. ** For Exponent

10.2.212.1. Traq

** For Exponent, in src/views/layouts/_footer.phtm:5.

pow(1024, 2) could be (1023 ** 2), to convert bytes into Mb.

<?=round((microtime(true) - START_TIME), 2); ?>s, <?php echo round((memory_get_peak_usage() - START_MEM) / pow(1024, 2), 3)?>mb

10.2.212.2. TeamPass

** For Exponent, in includes/libraries/Authentication/phpseclib/Math/BigInteger.php:286.

pow(2, 62) could also be hard coded with 0x4000000000000000.

pow(2, 62)

10.2.213. Could Use Compact

10.2.213.1. WordPress

Could Use Compact, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.214. Could Use array_fill_keys

10.2.214.1. ChurchCRM

Could Use array_fill_keys, in src/ManageEnvelopes.php:107.

There are two initialisations at the same time here : that should make two call to array_fill_keys().

foreach ($familyArray as $fam_ID => $fam_Data) {
        $envelopesByFamID[$fam_ID] = 0;
        $envelopesToWrite[$fam_ID] = 0;
    }

10.2.214.2. PhpIPAM

Could Use array_fill_keys, in functions/scripts/merge_databases.php:418.

Even when the initialization is mixed with other operations, it is a good idea to extract it from the loop and give it to array_fill_keys().

$arr_new = array();
                            foreach ($arr as $type=>$objects) {
                                    $arr_new[$type] = array();
                                    if(sizeof($objects)>0) {
                                            foreach($objects as $ok=>$object) {
                                                    $arr_new[$type][] = $highest_ids_append[$type] + $object;
                                            }
                                    }
                            }

10.2.215. Use Count Recursive

10.2.215.1. WordPress

Use Count Recursive, in wp-admin/includes/misc.php:74.

This code actually loads the file, join it, then split it again. file() would be sufficient.

$markerdata = explode( "\n", implode( '', file( $filename ) ) );

10.2.215.2. PrestaShop

Use Count Recursive, in controllers/admin/AdminSearchController.php:342.

This could be improved with count() recursive and a array_filter call, to remove empty $list.

$nb_results = 0;
            foreach ($this->_list as $list) {
                if ($list != false) {
                    $nb_results += count($list);
                }
            }

10.2.216. Should Use Foreach

10.2.216.1. ExpressionEngine

Should Use Foreach, in system/ee/EllisLab/ExpressionEngine/Service/Model/Query/Builder.php:241.

This code could turn the string into an array, with the explode() function, and use foreach(), instead of calculating the length() initially, and then building the loop.

$length = strlen($str);
            $words = array();

            $word = '';
            $quote = '';
            $quoted = FALSE;

            for ($i = 0; $i < $length; $i++)
            {
                    $char = $str[$i];

                    if (($quoted == FALSE && $char == ' ') || ($quoted == TRUE && $char == $quote))
                    {
                            if (strlen($word) > 2)
                            {
                                    $words[] = $word;
                            }

                            $quoted = FALSE;
                            $quote = '';
                            $word = '';

                            continue;
                    }

                    if ($quoted == FALSE && ($char == ' || $char == ") && ($word === '' || $word == '-'))
                    {
                            $quoted = TRUE;
                            $quote = $char;
                            continue;
                    }

                    $word .= $char;
            }

10.2.216.2. Woocommerce

Should Use Foreach, in includes/libraries/class-wc-eval-math.php:84.

This loops reviews the ‘stack’ and updates its elements. The same loop may leverage foreach and references for more efficient code.

$stack_size = count( $stack );
                            for ( $i = 0; $i < $stack_size; $i++ ) { // freeze the state of the non-argument variables
                                    $token = $stack[ $i ];
                                    if ( preg_match( '/^[a-z]\w*$/', $token ) and ! in_array( $token, $args ) ) {
                                            if ( array_key_exists( $token, self::$v ) ) {
                                                    $stack[ $i ] = self::$v[ $token ];
                                            } else {
                                                    return self::trigger( "undefined variable '$token' in function definition" );
                                            }
                                    }
                            }

10.2.217. Too Many Parameters

10.2.217.1. WordPress

Too Many Parameters, in wp-admin/includes/misc.php:74.

11 parameters is a lot for a function. Note that it is more than the default configuration, and reported there. This may be configured.

/**
 * [identifyUserRights description]
 * @param  string $groupesVisiblesUser  [description]
 * @param  string $groupesInterditsUser [description]
 * @param  string $isAdmin              [description]
 * @param  string $idFonctions          [description]
 * @return string                       [description]
 */
function identifyUserRights(
    $groupesVisiblesUser,
    $groupesInterditsUser,
    $isAdmin,
    $idFonctions,
    $server,
    $user,
    $pass,
    $database,
    $port,
    $encoding,
    $SETTINGS
) {

10.2.217.2. ChurchCRM

Too Many Parameters, in src/Reports/ReminderReport.php:192.

10 parameters is a lot for a function. Here, we may also identify a family (ID, Name), and a full address (Address1, Address2, State, Zip, Country), which may be turned into an object.

public function StartNewPage($fam_ID, $fam_Name, $fam_Address1, $fam_Address2, $fam_City, $fam_State, $fam_Zip, $fam_Country, $fundOnlyString, $iFYID)
{

10.2.218. Should Preprocess Chr

10.2.218.1. phpadsnew

Should Preprocess Chr, in phpAdsNew-2.0/adview.php:302.

Each call to chr() may be done before. First, chr() may be replace with the hexadecimal sequence “0x3B”; Secondly, 0x3b is a rather long replacement for a simple semi-colon. The whole pragraph could be stored in a separate file, for easier modifications.

echo chr(0x47).chr(0x49).chr(0x46).chr(0x38).chr(0x39).chr(0x61).chr(0x01).chr(0x00).
                 chr(0x01).chr(0x00).chr(0x80).chr(0x00).chr(0x00).chr(0x04).chr(0x02).chr(0x04).
                     chr(0x00).chr(0x00).chr(0x00).chr(0x21).chr(0xF9).chr(0x04).chr(0x01).chr(0x00).
                 chr(0x00).chr(0x00).chr(0x00).chr(0x2C).chr(0x00).chr(0x00).chr(0x00).chr(0x00).
                 chr(0x01).chr(0x00).chr(0x01).chr(0x00).chr(0x00).chr(0x02).chr(0x02).chr(0x44).
                 chr(0x01).chr(0x00).chr(0x3B);

10.2.219. Drop Substr Last Arg

10.2.219.1. SuiteCrm

Drop Substr Last Arg, in modules/UpgradeWizard/uw_utils.php:2422.

substr() is even trying to go beyond the end of the string.

substr($relativeFile, 1, strlen($relativeFile))

10.2.219.2. Tine20

Drop Substr Last Arg, in tine20/Calendar/Frontend/Cli.php:95.

Omitting the last character would yield the same result.

substr($opt, 18, strlen($opt))

10.2.220. Possible Increment

10.2.220.1. Zurmo

Possible Increment, in app/protected/modules/workflows/utils/SavedWorkflowsUtil.php:196.

There are suspicious extra spaces around the +, that give the hint that there used to be something else, like a constant, there. From the name of the methods, it seems that this code was refactored from an addition to a simple method call.

$timeStamp =  + $workflow->getTimeTrigger()->resolveNewTimeStampForDuration(time());

10.2.220.2. MediaWiki

Possible Increment, in includes/filerepo/file/LocalFile.php:613.

That is a useless assignation, except for the transtyping to integer that PHP does silently. May be that should be a +=, or completely dropped.

$decoded[$field] = +$decoded[$field]

10.2.221. One If Is Sufficient

10.2.221.1. Tikiwiki

One If Is Sufficient, in lib/wiki-plugins/wikiplugin_trade.php:152.

empty($params[‘inputtitle’]) should have priority over $params[‘wanted’] == ‘n’.

if ($params['wanted'] == 'n') {
            if (empty($params['inputtitle'])) {
                    $params['inputtitle'] = 'Payment of %0 %1 from user %2 to %3';
            }
    } else {
            if (empty($params['inputtitle'])) {
                    $params['inputtitle'] = 'Request payment of %0 %1 to user %2 from %3';
            }
    }

10.2.222. Could Use array_unique

10.2.222.1. Dolibarr

Could Use array_unique, in htdocs/includes/restler/framework/Luracast/Restler/Format/XmlFormat.php:250.

This loop has two distinct operations : the first collect keys and keep them unique. A combinaison of array_keys() and array_unique() would do that job, while saving the in_array() lookup, and the configuration check with ‘static::$importSettingsFromXml’. The second operation is distinct, and could be done with array_map().

$attributes = $xml->attributes();
            foreach ($attributes as $key => $value) {
                if (static::$importSettingsFromXml
                    && !in_array($key, static::$attributeNames)
                ) {
                    static::$attributeNames[] = $key;
                }
                $r[$key] = static::setType((string)$value);
            }

10.2.222.2. OpenEMR

Could Use array_unique, in gacl/gacl_api.class.php:441:441.

This loop is quite complex : it collects $aro_value in $acl_array[‘aro’][$aro_section_value], but also creates the array in $acl_array[‘aro’][$aro_section_value], and report errors in the debug log. array_unique() could replace the collection, while the debug would have to be done somewhere else.

foreach ($aro_value_array as $aro_value) {
                                    if ( count($acl_array['aro'][$aro_section_value]) != 0 ) {
                                            if (!in_array($aro_value, $acl_array['aro'][$aro_section_value])) {
                                                    $this->debug_text("append_acl(): ARO Section Value: $aro_section_value ARO VALUE: $aro_value");
                                                    $acl_array['aro'][$aro_section_value][] = $aro_value;
                                                    $update=1;
                                            } else {
                                                    $this->debug_text("append_acl(): Duplicate ARO, ignoring... ");
                                            }
                                    } else { //Array is empty so add this aro value.
                                            $acl_array['aro'][$aro_section_value][] = $aro_value;
                                            $update = 1;
                                    }
                            }

10.2.223. Too Many Children

10.2.223.1. Typo3

Too Many Children, in typo3/sysext/backend/Classes/Form/AbstractNode.php:26.

More than 15 children for this class : 15 is the default configuration.

abstract class AbstractNode implements NodeInterface, LoggerAwareInterface {

10.2.223.2. Woocommerce

Too Many Children, in includes/abstracts/abstract-wc-rest-controller.php:30.

This class is extended 22 times, more than the default configuration of 15.

class WC_REST_Controller extends WP_REST_Controller {

10.2.224. Should Use Operator

10.2.224.1. Zencart

Should Use Operator, in includes/modules/payment/paypal/paypal_curl.php:378.

Here, $options is merged with $values if it is an array. If it is not an array, it is probably a null value, and may be ignored. Adding a ‘array’ typehint will strengthen the code an catch situations where TransactionSearch() is called with a string, leading to clearer code.

function TransactionSearch($startdate, $txnID = '', $email = '', $options) {
    // several lines of code, no mention of $options
      if (is_array($options)) $values = array_merge($values, $options);
    }
    return $this->_request($values, 'TransactionSearch');
  }

10.2.224.2. SugarCrm

Should Use Operator, in include/utils.php:2093:464.

$override should an an array : if not, it is actually set by default to empty array. Here, a typehint with a default value of ‘array()’ would offset the parameter validation to the calling method.

function sugar_config_union( $default, $override ){
    // a little different then array_merge and array_merge_recursive.  we want
    // the second array to override the first array if the same value exists,
    // otherwise merge the unique keys.  it handles arrays of arrays recursively
    // might be suitable for a generic array_union
    if( !is_array( $override ) ){
            $override = array();
    }
    foreach( $default as $key => $value ){
            if( !array_key_exists($key, $override) ){
                    $override[$key] = $value;
            }
            else if( is_array( $key ) ){
                    $override[$key] = sugar_config_union( $value, $override[$key] );
            }
    }
    return( $override );
}

10.2.225. Could Be Static Closure

10.2.225.1. Piwigo

Could Be Static Closure, in include/ws_core.inc.php:620.

The closure function($m) makes no usage of the current object : using static prevents $this to be forwarded with the closure.

/**
   * WS reflection method implementation: lists all available methods
   */
  static function ws_getMethodList($params, &$service)
  {
    $methods = array_filter($service->_methods,
      function($m) { return empty($m["options"]["hidden"]) || !$m["options"]["hidden"];} );
    return array('methods' => new PwgNamedArray( array_keys($methods),'method' ) );
  }

10.2.226. Could Be Typehinted Callable

10.2.226.1. Magento

Could Be Typehinted Callable, in wp-admin/includes/misc.php:74.

$objMethod argument is used to call a function, a method or a localmethod. The typehint would save the middle condition, and make a better job than ‘is_array’ to check if $objMethod is callable. Yet, the final ‘else’ means that $objMethod is also the name of a method, and PHP won’t validate this, unless there is a function with the same name. Here, callable is not an option.

public function each($objMethod, $args = [])
    {
        if ($objMethod instanceof \Closure) {
            foreach ($this->getItems() as $item) {
                $objMethod($item, ...$args);
            }
        } elseif (is_array($objMethod)) {
            foreach ($this->getItems() as $item) {
                call_user_func($objMethod, $item, ...$args);
            }
        } else {
            foreach ($this->getItems() as $item) {
                $item->$objMethod(...$args);
            }
        }
    }

10.2.226.2. PrestaShop

Could Be Typehinted Callable, in wp-admin/includes/misc.php:74.

$funcname is tested with is_callable() before being used as a method. Typehint callable would reduce the size of the code.

public static function arrayWalk(&$array, $funcname, &$user_data = false)
    {
            if (!is_callable($funcname)) return false;

            foreach ($array as $k => $row)
                    if (!call_user_func_array($funcname, array($row, $k, $user_data)))
                            return false;
            return true;
    }

10.2.227. Add Default Value

10.2.227.1. Zurmo

Add Default Value, in wp-admin/includes/misc.php:74.

Default values may be a literal (1, ‘abc’, …), or a constant : global or class. Here, MissionsListConfigurationForm::LIST_TYPE_AVAILABLE may be used directly in the signature of the method

public function getMetadataFilteredByOption($option)
        {
            if ($option == null)
            {
                $option = MissionsListConfigurationForm::LIST_TYPE_AVAILABLE;
            }

10.2.227.2. Typo3

Add Default Value, in wp-admin/includes/misc.php:74.

$extension could get a default value to handle default situations : for example, a file is htm format by default, unless better known. Also, the if/then structure could get a ‘else’ clause, to handle unknown situations : those are situations where the extension is provided but not known, in particular when the icon is missing in the storage folder.

public function getIcon($extension)
    {
        if ($extension === 'htm') {
            $extension = 'html';
        } elseif ($extension === 'jpeg') {
            $extension = 'jpg';
        }
        return 'EXT:indexed_search/Resources/Public/Icons/FileTypes/' . $extension . '.gif';
    }

10.2.228. Named Regex

10.2.228.1. Phinx

Named Regex, in src/Phinx/Util/Util.php:127.

$matches[1] could be renamed by $matches[‘filename’], if the capturing subpattern was named ‘filename’.

const MIGRATION_FILE_NAME_PATTERN = '/^\d+_([\w_]+).php$/i';
//.... More code with class definition
    public static function mapFileNameToClassName($fileName)
    {
        $matches = [];
        if (preg_match(static::MIGRATION_FILE_NAME_PATTERN, $fileName, $matches)) {
            $fileName = $matches[1];
        }

        return str_replace(' ', '', ucwords(str_replace('_', ' ', $fileName)));
    }

10.2.228.2. shopware

Named Regex, in engine/Library/Enlight/Components/Snippet/Resource.php:207.

$_match[3] is actually extracted two preg_match() before : by the time we read its usage, the first regex has been forgotten. A named subpattern would be useful here to remember what was captured.

if (!preg_match("!(.?)(name=)(.*?)(?=(\s|$))!", $_block_args, $_match) && empty($_block_default)) {
                throw new SmartyException('"' . $_block_tag . '" missing name attribute');
            }
            $_block_force = (bool) preg_match('#[\s]force#', $_block_args);
            $_block_json = (bool) preg_match('#[\s]json=["\']true["\']\W#', $_block_args);
            $_block_name = !empty($_match[3]) ? trim($_match[3], '\'"') : $_block_default;

10.2.229. Could Use Try

10.2.229.1. Mautic

Could Use Try, in app/bundles/StageBundle/Controller/StageController.php:78.

$limit is read as a session variable or a default value. There are no check here that $limit is not null, before using it in a division. It is easy to imagine this is done elsewhere, yet a try/catch could help intercept unwanted situations.

//set limits
        $limit = $this->get('session')->get(
            'mautic.stage.limit',
            $this->coreParametersHelper->getParameter('default_pagelimit')
        );
/... Code where $limit is read but not modified /
        $count = count($stages);
        if ($count && $count < ($start + 1)) {
            $lastPage = ($count === 1) ? 1 : (ceil($count / $limit)) ?: 1;

10.2.230. Use Basename Suffix

10.2.230.1. NextCloud

Use Basename Suffix, in lib/private/URLGenerator.php:176.

This code removes the 4 last letters from the images. It may be ‘png’, ‘jpg’ or ‘txt’.

substr(basename($image), 0, -4)

10.2.230.2. Dolibarr

Use Basename Suffix, in htdocs/core/website.inc.php:42.

The extension ‘.tpl.php’ is dropped from the file name, unless it appears somewhere else in the $websitepagefile variable.

str_replace(array('.tpl.php', 'page'), array('', ''), basename($websitepagefile))

10.2.231. Don’t Loop On Yield

10.2.231.1. Dolibarr

Don’t Loop On Yield, in htdocs/includes/sabre/sabre/dav/lib/DAV/Server.php:912.

Yield from is a straight replacement here.

if (($newDepth === self::DEPTH_INFINITY || $newDepth >= 1) && $childNode instanceof ICollection) {
                foreach ($this->generatePathNodes($subPropFind) as $subItem) {
                    yield $subItem;
                }
            }

10.2.231.2. Tikiwiki

Don’t Loop On Yield, in htdocs/includes/sabre/sabre/dav/lib/DAV/Server.php:912.

The replacement with yield from``is not straigthforward here. Yield is only called when $user hasn't been ``$done : this is a unicity check. So, the double loop may produce a fully merged array, that may be reduced further by array_unique(). The final array, then, can be used with yield from.

$done = [];

                    foreach ($goal['eligible'] as $groupName) {
                            foreach ($userlib->get_group_users($groupName) as $user) {
                                    if (! isset($done[$user])) {
                                            yield ['user' => $user, 'group' => null];
                                            $done[$user] = true;
                                    }
                            }
                    }

10.2.232. Multiple Usage Of Same Trait

10.2.232.1. NextCloud

Multiple Usage Of Same Trait, in build/integration/features/bootstrap/WebDav.php:41.

WebDav uses Sharing, and Sharing uses Webdav. Once using the other is sufficient.

trait WebDav {
    use Sharing;

}
//Trait Sharing is in /build/integration/features/bootstrap/Sharing.php:36

10.2.233. Function Subscripting, Old Style

10.2.233.1. OpenConf

Function Subscripting, Old Style, in openconf/include.php:1469.

Here, $advocateid may be directly read from ocsql_fetch_assoc(), although, checking for the existence of ‘advocateid’ before accessing it would make the code more robust

$advocateid = false;
    if (isset($GLOBALS['OC_configAR']['OC_paperAdvocates']) && $GLOBALS['OC_configAR']['OC_paperAdvocates']) {
            $ar = ocsql_query(SELECT `advocateid` FROM ` . OCC_TABLE_PAPERADVOCATE . ` WHERE `paperid`=' . safeSQLstr($pid) . ') or err('Unable to retrieve advocate');
            if (ocsql_num_rows($ar) == 1) {
                    $al = ocsql_fetch_assoc($ar);
                    $advocateid = $al['advocateid'];
            }
    }