Static analysis of PHP code on the example of symfony2

annotation

The need for static analysis in large projects has already been written more than once and, mainly, with a focus on strongly typed languages, for example, here and here .

With PHP, the situation is more complicated: we have already written about static analysis of PHP code, but in general the tools are much poorer here, and the dynamic nature of the language makes the development and testing process more complicated. For comparison, in the same Java project compilation itself helps to find errors, and in PHP the typing is weak, so even tests can skip errors.

In our company, we partially solved this problem by including static analysis and strong typing in the process of developing and verifying changes. After that, I wanted to see what was happening in other projects.

Instruments

I’ll skip well-known utilities ( PHP Mess Detector , PHP Copy / Paste Detector , PHP CodeSniffer , PHP Analyzer , Pfff ) - we used them at a certain stage, but the results were not worth the effort, so we’ll go straight to the smart tools that will bring benefit almost immediately.
')
Theoretically, we could use SensioLabs Insight , but corporate rules do not allow giving the code to a third party. Thus, our basic requirement returned to the classical integration of static analysis in the IDE. In our case, this is PhpStorm (you can put a trial version for 30 days if you want to check your project). By default, a good set of static analysis rules is available, but this, frankly, is not enough for enterprise-level products, so we put the Php Inspections extension (EA Extended) . This extension is the main tool of our analysis, and all subsequent examples are to them.

What is symfony2

Symfony is a opensource framework written in PHP5 ( more on architecture ). Version 2.7 was selected for review.

Symfony2 code analysis

To analyze the code, we take only the kernel components that are in the src / Symfony / Component folder.

Our analysis found 6924 problems of varying degrees of severity and different categories (the extension contains about 40 inspections, but not all of them found problems).
For comparison, in version 2.3, there were 5727 problems (20% less), although this could be either new components or new tests.

Debriefing

It makes no sense to consider all cases, and there will be false positives, therefore, we will consider only a few interesting code fragments (in relation to the type of problem).

Problem Type - Architecture: class re-implements interface of a superclass

namespace Symfony\Component\Translation\Loader; //   class MoFileLoader extends ArrayLoader implements LoaderInterface { ... } //   class ArrayLoader implements LoaderInterface { ... } 

It is not clear: either the interface was added to the parent class later, or the developers simply overlooked the inheritance of classes, but re-inheriting the interface in the child class does not make sense.

Problem Type - Architecture: class overrides a field of superclass

 namespace Symfony\Component\Translation\Dumper; class IcuResFileDumper extends FileDumper { ... //   protected $relativePathTemplate = '%domain%/%locale%.%extension%'; ... } //   abstract class FileDumper implements DumperInterface { protected $relativePathTemplate = '%domain%.%locale%.%extension%'; ... } 

It was necessary to override the default value, which we see.

The problem is that the attribute is duplicated in the child area, although initially it is an attribute of the parent class. It should be implemented like this:

 use Symfony\Component\Translation\MessageCatalogue; class IcuResFileDumper extends FileDumper { ... public function __construct() { $this->relativePathTemplate = '%domain%/%locale%.%extension%'; } ... } 

Problem Type - Probable bugs: missing parent constructor / clone call

 namespace Symfony\Component\HttpFoundation; class FileBag extends ParameterBag { ... //   public function __construct(array $parameters = array()) { $this->replace($parameters); } ... public function replace(array $files = array()) { $this->parameters = array(); $this->add($files); } ... } //   class ParameterBag implements \IteratorAggregate, \Countable { ... public function __construct(array $parameters = array()) { $this->parameters = $parameters; } ... } 

In OOP, calling the parent constructor / destructor is a common practice. In this case, it seems that the developers simply overlooked the code. It should be implemented like this:

 class FileBag extends ParameterBag { ... public function __construct(array $parameters = array()) { parent::__construct(); $this->add($files); } ... } 

Problem Type - Control flow: loop which does not loop

 namespace Symfony\Component\Templating\Loader; class ChainLoader extends Loader { ... public function isFresh(TemplateReferenceInterface $template, $time) { foreach ($this->loaders as $loader) { //   return $loader->isFresh($template, $time); } return false; } ... } 

It is possible that this is the desired behavior of the method, but the class name and the loop indicate rather an error — it will always be checked only at the first iteration.

Problem Type - Control flow: ternary operator simplification

 class CrawlerTest extends \PHPUnit_Framework_TestCase { ... public function testReduce() { ... $nodes = $crawler->reduce(function ($node, $i) { //   return $i == 1 ? false : true; }); ... } ... } 

This, of course, is not a mistake, but there is not much point in this code either.
In the test fragment, the return can be simplified like “return $ i! = 1;”.

Problem Type - Performance: elvis operator can be used

 namespace Symfony\Component\HttpKernel\DataCollector; class RequestDataCollector extends DataCollector implements EventSubscriberInterface { ... public function collect(Request $request, Response $response, \Exception $exception = null) { ... $this->data = array( ... //   'content_type' => $response->headers->get('Content-Type') ? $response->headers->get('Content-Type') : 'text/html', ... } ... } 

This is also not an error, but a simplification of the code, and can be rewritten as "'content_type' => $ response-> headers-> get ('Content-Type')?: 'Text / html'".
Basically, the new Symfony2 code is already using this statement, and this piece of code has simply not been noticed yet.

AlexPTS suggested a very simple option:

  $response->headers->get('Content-Type', 'text/html') 

Problem Type - Control flow: not optimal if conditions

This is a separate class of errors and non-optimal code. There are quite a few variations of problems, since conditional structures in legacy projects and after active refactoring are the headache of any architect.

Here are a couple of examples of what this analyzer finds.

 namespace Symfony\Component\HttpKernel\Profiler; abstract class BaseMemcacheProfilerStorage implements ProfilerStorageInterface { ... public function find($ip, $url, $limit, $method, $start = null, $end = null) { ... //   if ($ip && false === strpos($itemIp, $ip) || $url && false === strpos($itemUrl, $url) || $method && false === strpos($itemMethod, $method)) { ... } ... } ... } 

A vivid example of how to confuse everyone at once. In fact, the structure is as follows:

  ($ip && false === strpos($itemIp, $ip)) || ($url && false === strpos($itemUrl, $url)) || ($method && false === strpos($itemMethod, $method)) 

Duplicate calls to methods and functions:

 namespace Symfony\Component\Form\Extension\Core\DataMapper; class PropertyPathMapper implements DataMapperInterface { ... public function mapFormsToData($forms, &$data) { ... //   if ($form->getData() instanceof \DateTime && $form->getData() == $this->propertyAccessor->getValue($data, $propertyPath)) { ... } if (!is_object($data) || !$config->getByReference() || $form->getData() !== $this->propertyAccessor->getValue($data, $propertyPath)) { $this->propertyAccessor->setValue($data, $propertyPath, $form->getData()); } ... } } 

"$ form-> getData ()" is called several times, although it would be more logical to save the result in a local variable.

Instead of conclusion

Static code analysis (including PHP code) is a powerful and useful practice, albeit costly from the standpoint of organizing the development and training of teams.

But with daily use, this practice works very effectively:

• reduces the number of errors;
• helps to objectively approach the selection of components for projects;
• evaluate the new project and understand from which side to approach it;
• tighten the level of their teams.

On the example of opensource, all these items are just as relevant as in the enterprise, just a quick glance at the problems found.

For PHP, the situation with static analysis tools has improved significantly over the last couple of years and seems to be improving further.