Company, Technology

Exakat 1.2.3 review

Posted on by dams

<

Exakat 1.2.3 review

Exakat 1.2.3 is steadily bringing more analysis and reports to PHP coders. This week, we are excited to see the start of the new ‘Manual’ report, and three new analysis, that checks strtr() usage, reports property unsetting and list all complex expressions. So, “call me Exakat”: here is the Exakat 1.2.1 review.

New report: Code Manual

Static analysis helps you document your code as you write it. Exakat is great for spotting bugs in the code. Along the way, it has to document various features, so as to reduce the false-positive. In the end, that documentation is also precious for manual audits.

Exakat 1.2.3 introduces the new ‘Manual’ report. This report is a MD file, which collects interesting informations, read in the code.

  • Constant names and their value
  • Exception Tree
  • URL
  • Email
  • SQL
  • Regex
  • Incoming variables
  • Dynamic expressions
  • Error messages

Most of the time, those pieces of information are scattered across the code, leading to the infamous ‘Magic number’ syndrom. The ‘Magic number’ syndrom is those literal numbers that appear in the code, without explanation. When those numbers have more purpose than their face value, they should be turned into a constant. With a name, it is clearer, even without documentation.

By extension, the manual report collects interesting pieces of information, helping their reuse. Regular expressions, for examples, tend to be hardcoded every time we need them, and then, updated one by one, when a change has to be applied. With all the regex at the same place, it is easy to review them, and realize that it has already been written, and it is actually used in many places in the code. More consistence, less coding.

One great advantage of static analysis documentation is its speed of update: commit any change in the code, and the documentation will be updated on the fly. No detour to log anything, nor update a current documentation: the doc may be updated immediately.

This first version of the Manual report has nine sections: we already have many ideas to implement many other interesting sections. Feel free to suggest new features, on github.com/exakat/exakat or on the slack channel!

Check strtr usage

strtr is one of the top 100 PHP functions of all time: it’s been with PHP since PHP 3 (probably even 2.0). It replaces characters in a piece of string.

<?php 
  echo strtr('caché', 'âé', 'ae'); 
?>

strtr only replaces characters on the spot. This leads to some interesting constraints on the arguments: when using strings as arguments, both string should be of the same size. The shorter string will be used.

And also, strtr can’t remove characters, since the empty string is always the shortest.

<?php 
// fg are not used here 
echo strtr('caché', 'âé', 'aefg'); 

// ch are not used here 
echo strtr('caché', 'âéch', 'ae'); 

// no change in the input string 
echo strtr('caché', 'âé', ''); 

?>

One final idea for strtr: it is actually more efficient than strreplace or pregreplace. But it is also a lot simpler, changing character one by one. We could add a new rule suggesting the usage of strtr over the others, when the change or the regex are simple enough.

Echo / print / printf preference

Upon the suggestion of Jan-Willem van Schie (@jwvanschie), we added ‘printf’ to the ‘echo / print’ preference. printf is usually less used than echo/print: unless formatting the variables is important, echo and printf are more convenient. We’ll see if that preference emerge as a dominant one in the coming weeks.

Don’t unset properties

Last week AFUP meeting in Paris yielded an interesting trap: PHP allows to to destroy a property. Benoit Burnichon (@benoitburnichon) mentioned loosing a lot of time, looking for an unset property. The property was defined in the class definition, but later disappeared.

<?php 
class A { 
  public $a = 1; 
  function cleanProperty(){ 
    // $this->a = null;
       unset($this->a);
    }
}

$a = new A();
print_r((array) $a);
$a->cleanProperty();
print_r((array) $a);

?>

A better coding behavior is nulling the property: assigning null to the property keeps the property defined. Yet, it will not yield Undefined property: A::$b when used, but it will still yield Trying to get property 'c' of non-object when used as another object, or so. Testing it with isset() or empty() is also warning-less and behaves correctly.

After creating this new analysis, we started reviewing code that produced issues based on this. It appears that many classes first unset the property, then restore its initial definition, like this:

<?php 
  public function clearNotificationQueue() { 
        unset($this->_NotificationQueue);
        $this->_NotificationQueue = [];
    }
?>

_NotificationQueue is actually defined as an empty array(). Here, it is unset, then reset to its default value. It seems we can skip the unset, and directly set a new unrelated value. The GC will happen in time to clean any object here. Anyone has better experience on the subject? It may be an old PHP 4 behavior that stuck until now.

All your complex expressions are belongs to us

The longer they are, the harder the expression are to read. While we strive to keep our methods short, it happens that some expression get wild and grow tremendously.

Think about concatenations, for example. Up to seven variables, they may be still readable. You may even find ways to keep them readable even past that number, though it may not be for everyone.

On the other hand, when the same-sized concatenation includes as many PHP native calls, that makes a really difficult to read expression. Take this

@unlink(getcwd( ) . substr($fileFrom, strlen($unzip_dir), strlen($fileFrom)))

 

It only includes 5 PHP native calls : unlink, substr, getcwd and strlen (twice). Although, the accumulation of those makes the whole expression difficult to read, and very little errors are handled.

After a first check, it appears that most expressions include up to 4 PHP native function calls. Expressions using more than 5 native functions calls are getting very rare, and may be reported safely. It also appears that a lot of them are printing data, like this code :

@print (preg_replace('#/\*\s*\*/\s*#', '', str_replace('url(\'\')', 'none', str_replace('url("")', 'none', preg_replace('#\{\$[^\}]*\}#', '', file_get_contents($FILE_BASE . '/themes/default/css/global.css'))))))

Exakat is now reporting those expressions, in order to help with readability.

The bulk of such long expression is made of long concatenations and fluent interfaces usage. For example:

$key = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('tx_rsaauth_keys')->select(['key_value'], 'tx_rsaauth_keys', ['uid' => $keyToBeDeleted])->fetchColumn( )

$runAction = '<a class="btn btn-default" data-toggle="tooltip" data-container="body" href="' . htmlspecialchars($this->moduleUri . '&tx_scheduler[execute][]=' . $schedulerRecord['uid']) . '" title="' . htmlspecialchars($this->getLanguageService( )->getLL('action.run_task')) . '">' . $this->moduleTemplate->getIconFactory( )->getIcon('extensions-scheduler-run-task', Icon::SIZE_SMALL)->render( ) . '</a>'

Indeed, with a good IDE, such expressions are not so difficult to read. Yet, they tend to attract more errors than shorter expressions, as jumping from one technology to the other (HTML to PHP to SQL, for example), is error prone.

On the other hand, some expression are truly gruesome :

$this->defaultLinkTarget = $this->classesAnchorDefault[$this->displayedLinkHandlerId] && 
$this->classesAnchorDefaultTarget[$this->displayedLinkHandlerId] ? 
$this->classesAnchorDefaultTarget[$this->displayedLinkHandlerId] :
 (isset($this->buttonConfig[$this->displayedLinkHandlerId]['properties']['target']['default']) ?
 $this->buttonConfig[$this->displayedLinkHandlerId]['properties']['target']['default'] :
 (isset($this->buttonConfig['properties']['target']['default']) ?
 $this->buttonConfig['properties']['target']['default'] : ''))

Happy PHP code reviews

All the 344 analyzers are presented in the docs, including the agreeable Logical To in_array: long lists of logical comparison may be replaced by one inarray() call. The code is more readable and execution is faster. It’s a popular PHP bug <https://178.62.231.40/, rating at 49%.

You can check all of the exakat reports at the gallery: exakat gallery.

Download Exakat on exakat.io, install it with Docker, upgrade it with ‘exakat.phar upgrade -u’ and like us on github.