Статический анализ PHP кода на примере Symfony2

Аннотация

О необходимости статического анализа в больших проектах уже писали не раз и, в основном, с фокусом на строго типизированные языки, например, здесь и здесь.

С PHP дело обстоит сложнее: уже писали про статический анализ PHP кода, но в целом инструментарий тут гораздо беднее, и динамическая природа языка делает процесс разработки-тестирования сложнее. Для сравнения, в той же Java компиляция проекта сама по себе помогает найти ошибки, а в PHP типизация слабая, поэтому даже тесты могут пропустить ошибки.

В нашей компании мы эту проблему частично решили, включив статический анализ и строгую типизацию в процесс разработки и проверки изменений. После этого мне захотелось посмотреть, что происходит в других проектах.

Инструменты

Я пропущу хорошо известные утилиты (PHP Mess Detector, PHP Copy/Paste Detector, PHP CodeSniffer, PHP Analyzer, Pfff), — мы их использовали на определённом этапе, но результаты не стоили приложенных усилий, — поэтому перейдём сразу к умным инструментам, которые принесут пользу почти сразу.

Теоретически мы могли бы использовать SensioLabs Insight, но корпоративные правила не позволяют отдавать код третьей стороне. Таким образом, наше основное требование вернулось к классической интеграции статического анализа в IDE. В нашем случае это PhpStorm (можно поставить пробную версию на 30 дней, если хочется проверить свой проект). По умолчанию доступен неплохой набор правил статического анализа, но этого, честно говоря, недостаточно для продуктов enterprise-уровня, поэтому ставим расширение Php Inspections (EA Extended). Это расширение — основной инструмент нашего анализа, и все последующие примеры находятся именно им.

Что такое Symfony2

Symfony — это opensource-фреймворк, написанный на PHP5, и использующий шаблон проектирования MVC. Для обзора была выбрана актуальная версия 2.7 (LTS).

Анализ кода Symfony2

Для анализа кода возьмём только компоненты ядра, находящиеся в папке src/Symfony/Component.

Наш анализ нашёл 6924 проблемы разной степени тяжести и разных категорий (расширение содержит около 40 инспекций, но не все из них нашли проблемы).
Для сравнения, в версии 2.3 нашлось 5727 проблем (на 20% меньше), хотя это могут быть как новые компоненты, так и новые тесты.

Разбор проблем

Рассматривать все случаи смысла нет, да и ложные срабатывания будут, поэтому рассмотрим только несколько интересных фрагментов кода (в привязке к типу проблемы).

Тип проблемы — Architecture: class re-implements interface of a superclass

namespace SymfonyComponentTranslationLoader;

// проблема здесь
class MoFileLoader extends ArrayLoader implements LoaderInterface {
    ...
}

// родительский класс
class ArrayLoader implements LoaderInterface {
    ...
}

Непонятно: или интерфейс внесли в родительский класс позже, или просто разработчики недоглядели за наследованием классов, но повторное наследование интерфейса в дочернем классе не имеет смысла.

Тип проблемы — Architecture: class overrides a field of superclass

namespace SymfonyComponentTranslationDumper;

class IcuResFileDumper extends FileDumper {
    ...
    // проблема здесь
    protected $relativePathTemplate = '%domain%/%locale%.%extension%';
    ...
}

// родительский класс
abstract class FileDumper implements DumperInterface {
    protected $relativePathTemplate = '%domain%.%locale%.%extension%';
    ...
}

Необходимо было переопределить значение по умолчанию, что мы и видим.

Проблема в том, что атрибут продублирован в дочернюю область, хотя изначально это атрибут родительского класса. Следовало бы реализовать так:

use SymfonyComponentTranslationMessageCatalogue;

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

Тип проблемы — Probable bugs: missing parent constructor/clone call

namespace SymfonyComponentHttpFoundation;

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;
    }
    ...
}

В ООП вызов родительского конструктора/деструктора — это общепринятая практика. В данном случае похоже, что разработчики просто недосмотрели за кодом. Следовало бы реализовать так:

class FileBag extends ParameterBag {
    ...
    public function __construct(array $parameters = array()) {
        parent::__construct();

        $this->add($files);
    }
    ...
}

Тип проблемы — Control flow: loop which does not loop

namespace SymfonyComponentTemplatingLoader;

class ChainLoader extends Loader {
    ...
    public function isFresh(TemplateReferenceInterface $template, $time) {
        foreach ($this->loaders as $loader) {
            // проблема здесь
            return $loader->isFresh($template, $time);
        }

        return false;
    }
    ...
}

Возможно, что это желаемое поведение метода, но имя класса и цикл указывают скорее на ошибку — всегда будет выполняться проверка только на первой итерации.

Тип проблемы — 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;
        });
        ...
    }
    ...
}

Это, конечно, не ошибка, но особого смысла в таком коде тоже нет.
Во фрагменте теста возврат можно упростить так «return $i != 1;».

Тип проблемы — Performance: elvis operator can be used

namespace SymfonyComponentHttpKernelDataCollector;

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',
         ...
    }
    ...
}

Это тоже не ошибка, а упрощение кода, и можно переписать как "'content_type' => $response->headers->get('Content-Type') ?: 'text/html'".
В основном, новый код Symfony2 уже использует этот оператор, а данный фрагмент кода ещё просто не заметили.

Тип проблемы — Control flow: not optimal if conditions

Это отдельный класс ошибок и неоптимального кода. Вариаций проблем довольно много, т.к. условные структуры в легаси проектах и после активного рефакторинга — это головная боль любого архитектора.

Вот, пара примеров того, что находит этот анализатор.

namespace SymfonyComponentHttpKernelProfiler;

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)) {
                ...
            }
            ...
     }
     ...
}

Яркий пример, как запутать всех и сразу. На самом деле, структура следующая:

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

Дублирующие вызовы методов и функциий:

namespace SymfonyComponentFormExtensionCoreDataMapper;

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()" вызывается несколько раз, хотя логичнее было бы сохранить результат в локальную переменную.

Вместо заключения

Статический анализ кода (в том числе PHP кода) — мощная и полезная практика, хотя и дорогостоящая с позиции организации процесса разработки и обучения команд.

Но при повседневном использовании эта практика работает очень эффективно:

• уменьшает количество ошибок;
• помогает объективно подойти к выбору компонентов для проектов;
• оценить новый проект и понять, с какой стороны к нему подойти;
• подтянуть уровень своих команд.

На примере opensource все эти пункты так же актуальны, как и в enterprise, достаточно бегло взглянуть на найденные проблемы.

Для PHP ситуация с инструментарием для статического анализа за последние пару лет заметно улучшилась и, похоже, что будет улучшаться и дальше.

Автор: kalessil

Источник