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. Used Once Variables

10.2.2.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.2.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.3. Several Instructions On The Same Line

10.2.3.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. With a quick review, 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.3.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.4. Dangling Array References

10.2.4.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.4.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.5. Logical Should Use Symbolic Operators

10.2.5.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.5.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.6. Deep Definitions

10.2.6.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.7. Buried Assignation

10.2.7.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.7.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.8. No array_merge() In Loops

10.2.8.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.9. Useless Parenthesis

10.2.9.1. 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.10. Use Constant As Arguments

10.2.10.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.10.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.11. Could Be Static

10.2.11.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.11.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.12. Could Use Short Assignation

10.2.12.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.12.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.13. Timestamp Difference

10.2.13.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.13.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.14. Wrong Parameter Type

10.2.14.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.15. Identical Conditions

10.2.15.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 to simply calling $theme->parent(), as $has_template is unused here.

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

10.2.15.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.15.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.16. No Choice

10.2.16.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.16.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.17. Should Use Coalesce

10.2.17.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.17.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.18. Throw Functioncall

10.2.18.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.19. Cast To Boolean

10.2.19.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.19.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 simply cast to (bool).

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

10.2.20. Failed Substr Comparison

10.2.20.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.20.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.21. Don’t Echo Error

10.2.21.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.21.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.22. Too Many Local Variables

10.2.22.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.23. Only Variable Passed By Reference

10.2.23.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. Minimaly, the intermediate result must be saved in a variable, to be 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.23.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 faild. It is not possible to initialize variable AND use them as argument.

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

10.2.24. Assign With And

10.2.24.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.25. Logical To in_array

10.2.25.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.26. Could Be Private Class Constant

10.2.26.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.27. Next Month Trap

10.2.27.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.27.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.28. Identical On Both Sides

10.2.28.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.29. Redefined Private Property

10.2.29.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.30. Don’t Unset Properties

10.2.30.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.30.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.31. Strtr Arguments

10.2.31.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.32. Weak Typing

10.2.32.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.33. __debugInfo() Usage

10.2.33.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.34. Exception Order

10.2.34.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.35. Join file()

10.2.35.1. WordPress

Join 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.35.2. SPIP

Join 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.35.3. ExpressionEngine

Join 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.35.4. PrestaShop

Join 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.36. No Count With 0

10.2.36.1. 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.37. Use pathinfo() Arguments

10.2.37.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.37.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.38. Compare Hash

10.2.38.1. Traq

Compare Hash, in /src/Models/User.php:105.

This code should also avoid using SHA1.

sha1($password) == $this->password

10.2.38.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.39. Register Globals

10.2.39.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.39.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.40. Use List With Foreach

10.2.40.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.40.2. Swoole

Use List With Foreach, in libs/Swoole/SelectDB.php:848.

This foreach reads ‘c’ in the $c variable (via the $_c). It could be simplified with foreach($c as [‘c’ => $d]) { $cc += $d; }. In fact, it could very well be replaced by array_sum() altogether.

$cc = 0;
            foreach ($c as $_c)
            {
                $cc += $_c['c'];
            }