Пользователь
0,0
рейтинг
27 января 2015 в 15:20

Разработка → Статический анализ 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 (подробнее об архитектуре). Для обзора была выбрана версия 2.7.

Анализ кода Symfony2


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

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

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


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

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


namespace Symfony\Component\Translation\Loader;

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

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

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

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

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

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

use Symfony\Component\Translation\MessageCatalogue;

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


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

В ООП вызов родительского конструктора/деструктора — это общепринятая практика. В данном случае похоже, что разработчики просто недосмотрели за кодом. Следовало бы реализовать так:
class FileBag extends ParameterBag {
    ...
    public function __construct(array $parameters = array()) {
        parent::__construct();

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


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

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

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

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

AlexPTS подсказал совсем простой вариант:
    $response->headers->get('Content-Type', 'text/html')


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


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

Вот, пара примеров того, что находит этот анализатор.
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)) {
                ...
            }
            ...
     }
     ...
}

Яркий пример, как запутать всех и сразу. На самом деле, структура следующая:
    ($ip && false === strpos($itemIp, $ip)) 
    || 
    ($url && false === strpos($itemUrl, $url)) 
    || 
    ($method && false === strpos($itemMethod, $method))


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

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


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

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

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

Для PHP ситуация с инструментарием для статического анализа за последние пару лет заметно улучшилась и, похоже, что будет улучшаться и дальше.
Vladimir Reznichenko @kalessil
карма
15,0
рейтинг 0,0
Пользователь
Реклама помогает поддерживать и развивать наши сервисы

Подробнее
Спецпроект

Самое читаемое Разработка

Комментарии (56)

  • 0
    Resharper inspections в PHP варианте! Обязательно проверю свой проект.
    • 0
      До ReSharper далековато (спасибо за наводку), но подход тот же.
  • +4
    Хм… Все «проблемы», которые были найдены — их даже проблемами-то трудно назвать. Это скорее не проблемы, а просто предложения анализатора по приведению некоторых строчек кода в более хороший вид, за исключением пары случаев. Разве нет? :)
    • 0
      Ну оно же как-то работает) Шутка, Symfony2 хороший продукт, и там трудно найти эпичные ошибки (хотя тот же пример с ChainLoader или конструкторами).

      Остальные примеры — это улучшение кода, да. Я просто ещё опустил много мелочи: производительность, возможные баги и прочее.
    • 0
      Ну тогда вот — небольшой список. То, что выделено (кроме приведения типов) может оказаться багами:
      небольшой список
      image

  • +2
    спасибо за наводку на Php Inspections (EA Extended)
    • 0
      Пожалуйста
  • 0
    Хм… спасибо, не знал что есть такое расширение, хотя уже давно пользуюсь Штормом. Надо будет попробовать.
    На самом деле подобные анализы редко выявляют какие-либо ошибки, но если избавить код от мелких проблем (как в статье, например), о он реально становится лучше.
    • 0
      Скажем так, не напрямую. Но и забытых вар дампов, странных логических конструкций мы исправили достаточно.
  • +1
    Отличное исследование. Следующий шаг — собрать эти правки и оформить в pull request :)
    • +1
      «А вы сообщили разработчикам о найденных ошибках?»
      • +3
        Так всё же просто — fork, push, pull request, а там посмотрим, как отреагируют. Вообще, по моему опыту, мейнтейнеры совершенно нормально относятся даже к небольшим правкам, так что можете смело предлагать изменения. Сейчас, конечно, очередь достаточно большая, но когда это мешало?
        • +1
          Я честно пытался отшутиться, но у меня в контракте есть ограничения на участие в оперсорсе и выступлениях на конференциях.
      • 0
        Видать основная масса PHP программистов не интересуется C++, поэтому вашу шутку, относящуюся к PVS-Studio, не поняли :)
  • +1
    Для обзора была выбрана актуальная версия 2.7 (LTS)

    Дык 2.7 еще ж не релизилась. Или я чего-то не знаю?
    • 0
      Поправил, спасибо за ремарку.
  • 0
    Подскажите пожалуйста, как использовать данное расширение?
    Установил на PhpStom 8.0.2, перезапустил, в списке плагинов активен, а как запустить анализ? Стандартная команда Run Inspections прогоняет только стандартные инспекции, настроек не нашёл.
    • 0
      File > Settings > Inspections, в разделе «PHP» без групп:

      Заголовок спойлера

    • 0
      На маках нашлась проблема — возможно, как раз ваш случай. Скоро поправят.
      • 0
        Скорее всего, именно Apple JDK 1.6 у меня. Спасибо!
      • 0
        Поправили, наконец-то заработало в версии 1.1.0!
  • 0
    Если кликнуть правой кнопкой по файлам/папке в навигаторе проекта, то в контекстном меню есть опция «Inspect Code...» — можно часть проекта проверить.

    В результатах анализа будет секция PHP — это результаты проверки плагина.
  • –6
    Я вот что не понимаю. Зачем PHPшники так отчаянно превращают PHP в Java? Почему бы не делать на PHP исконно PHPшные задачи, а для статики использовать уже существующую Java? Зачем нам две джавы?
    • +4
      Наверное это начали сами разработчики PHP, когда при реализации в качестве основных источников вдохновения посматривали как раз таки на Java/c#.

      В любом случае, статический анализ и прочие полезности никакого отношения к Java не имеют. Статический тайпхинтинг — так же, это лишь удобности для разработчика.
      • –3
        а желание статического тайпхинтинга никак не умаляет динамической природы языка? Не получится ли так, что люди начнут писать «статические» программы на PHP?

        С одной стороны, в тексте приводятся довольно невинные примеры. С другой, статья с каких слов начинается: «о необходимости статического анализа в больших проектах уже писали не раз». Именно «необходимости». Это уже звоночек. Если начать всё вот так генерализировать, этот звоночек в конце концов может привести к тому, что заказчики будут требовать статически анализируемый код на PHP. То есть нельзя будет использовать его для того, для чего он собственно и нужен — крутой динамической магии.

        Это не про «прямо сейчас», а про развитие на годы вперед, куда это всё придет. Гадание что будет, что случится, чем сердце успокоится, дальняя дорога или казенный дом.
        • 0
          Тайпхинтинг никак вообще не должен мешать вам творить вашу «крутую динамическую магию». Статический же анализ в рамках динамического языка все-равно необходим (и не важно, пишите вы на PHP или Ruby), так как это значительно упрощает отлов глупых ошибок.
          • 0
            Например, магический метод, который обращение к несуществующему полю превращает в запрос к базе данных, и назад возвращает совершенно что угодно, что там в базе нашлось. Переменные переменных. Рекурсивные инклуды. Препроцессоры типа CCPP. Посмотрим, как с таким кодом справится тайпчекер, он же совершенно unsafe. Тайпчекер-то мне не помешает, это я ему случайно могу помешать, сводя всю работу к нулю.

            А если уж нужна статика, зачем ее тащить в PHP, если можно взять язык с таким же синтаксисом, в котором уже всё есть? Те же Java и C#. В Java динамику, кстати, не тащат совсем. Когда понадобилась динамика, просто создали другие языки на той же виртуальной машине. Более того, в саму виртуальную машину джавы добавили поддержку JavaScript, прямо в базовом наборе инструментов. Может, имеет смысл сделать другой язык для PHPшной виртуальной машины, если так нужна PHPшная виртуальная машина?
            • 0
              PHP был создан для решения задач разработки web-приложений максимально быстро а не для магии. Я не говорю что магией пользоваться совсем так уж плохо, просто это не особо хорошо в рамках реализации бизнес-логики, отладки, поддержки приложения. С этой точки зрения тайпхинтинг, статический анализ в купе с автотестами и прочими приблудами существенно упрощают сопровождение проекта.

              Вам случалось тратить кучу времени на отладку, и потом осознавать что вы где-то сделали опечатку в названии поля тех самых волшебных классов? Даже если использовать исключительно phpdoc и статический анализ встроенный в тот же phpstorm, такие ошибки находятся очень быстро.

              Магия скажем очень помогает при реализации каких-то вещей в духе прокси-классов и т.п. И да, магия основанная на рефлексии и кодо-генерации меня лично радует намного больше.
              • 0
                Почему вы тогда не возьмете языки, построенные на статике, рефлексии, кодогенерации и других милых вашему сердцу вещах? Например, упомянутые Java, C#. Там уже все это есть, все отлично работает, сто лет как.

                Мне в пхп хочется видеть наоборот больше динамики, раз уж в нем она возможна. Это дает выбор инструментов. Нужно на динамике быстро наколбасить прототип — пхп. Нужна динамика с прототипами — жс. Нужна статика — джава. Нужна статика под виндовс — сишарп. Итп. Для каждой задачи — свой инструмент. Сейчас же я вижу, что пхп превращается в джаву, то есть становится на один инструмент меньше. Если пхп похож на джаву, нет совершенно никакого смысла выбирать пхп, он умер. Это печально, в этом классе остается только руби.
                • +2
                  Как-то вы узко мыслите.
                  Те возможности который предоставляет PHP куда больше чем «на динамике быстро наколбасить прототип». PHP даёт возможность писать, в том числе, большие и сложные системы, при этом при написании таких систем уже применяются другие практики, в том числе использование тайпхинтинга где это возможно и т.д. И это не делает язык похожим на другие языки, это подход схож с другими подходами на других языках. И это не лишает язык его преимуществ. И при выборе языка для решения какой-то задачи критериев для выбора куда больше чем нудна статика или динамика.
                • +3
                  Только ситхи все возводят в абсолют.
  • 0
    Это тоже не ошибка, а упрощение кода, и можно переписать как "'content_type' => $response->headers->get('Content-Type') ?: 'text/html'".

    $response->headers->get('Content-Type', 'text/html') так не лучше?
    • 0
      Да, так ещё лучше, поправил.
  • +2
    включив строгую типизацию в процесс разработки

    А как вы это сделали? Перешли на hack?
    • 0
      По многим причинам Hack/HHVM мы не можем использовать.

      Костыли пришлось писать для примитивных типов (статика — необходимое зло в данном случае):

      class ..._SimpleTypeHint {
          public static function throwExceptionIfArgumentIsNotString($givenArgument);
          public static function throwExceptionIfArgumentIsNotInteger($givenArgument);
          public static function throwExceptionIfArgumentIsNotFloat($givenArgument);
          public static function throwExceptionIfArgumentIsNotBoolean($givenArgument);
      }
      
      class ..._SimpleTypeCasting {
          public static function toInteger($mixedValueToCast);
          public static function toFloat($mixedValueToCast);
          public static function toBoolean($mixedValueToCast);
      }
      


      А вот так выглядит худший случай использования:

      class SomeClass {
      	/** @var int|null $someId */
          public function doSomethingWithEntityId($someId)
          {
              if ($someId !== null) {
                  ..._SimpleTypeHint::throwExceptionIfArgumentIsNotInteger($someId);
              }
      		
      		...
      	}
      }
      
      • 0
        Остальные типы (массивы, классы и интерфейсы), типо-зависимые операции сравнения и поиска — часть языка и родного API.
  • +1
    … и использующий шаблон проектирования MVC ...

    Symfony2 не использует шаблон проектирования MVC, поправьте, пожалуйста.
    • +1
      Ваша правда, Фибиан об этом писал.
  • 0
    Посмотрел на расширение и увидел там «un-necessary double quotes» и даже грустно стало :(
    • 0
      Будем надеяться, что английский в названиях и сообщениях поправят.
      • 0
        Да не, я скорее про то, что двойные кавычки или одинарные — не важно
        • 0
          Пока там нет инлайн-переменных, то разницы нет.

          Если строка заключена в одинарные кавычки, то PHP не будет даже искать переменные (и спец. символы тоже) внутри — это главный нюанс. Теоретически объявление такой строки быстрее.

          Я правда не помню, что происходит внутри APC, там разница будет минимальна.
          • 0
            Opcache на то и опкеш, чтобы кешировать байткод.
            Весь этот поиск будет произведен во время компиляции, то есть один раз. Этой микрооптимизацией можно смело пренебрегать направо и налево, потому что это не важно — у всех включен опкеш.
            • 0
              Начиная с php5.4 все чуть веселее. Все строки (и хэши для ключей массивов) php старается инициализировать только один раз и затем шарит это все в рамках одного процесса и затем просто реюзает. За счет этого нам не приходится перевыделять память да и сама память существенно экономится. opcache просто дает возможность шарить все в рамках всего пула процессов делая оптимизацию еще более эффективной.

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

              Вообще большая часть этих холиварных вопросов типа «что быстрее echo или print» на корню решаются opcache.
            • 0
              Fesor очень хорошо ответил, мне нечего добавить
              • 0
                Не нужны тут никакие варианты, можно забить на эту проверку.
                Если вас она так волнует — вам надо в панике убегать с Symfony, потому что весь кеш шаблонов Twig на двойных кавычках :)
                • 0
                  Забивайте :)
    • 0
      А можете рассказать, чем вы пользуетесь для статического анализа и как?

      Доводы по поводу кавычек были конструктивны, хочу продолжить дискуссию в более интересном русле.
      • 0
        Все теми же инспекциями в phpstorm, поэтому и заинтересовался плагином из топика и с удовольствием его попробую :)
        • 0
          О, расскажите потом (что отключили, что оставили)?

          Хочу сравнить с нашим опытом (мы пока всё используем, но была пара предложений от коллег).
          • 0
            Могу посоветовать на данном этапе внедрить какую-то систему аналитики — мониторить что люди отключают и т.д. что бы выборка была более репрезентативна.
            • 0
              Платформа не даёт таких инструментов, плагин может только экспортировать инспекции и аста-лависта. Второй момент — я немного параноик на тему сбора статистики плагинами от стороннего вендора.
          • 0
            У нас в команде пока нет каких-то жестко формализованных правил писать код, потихоньку к этому идем, так что и общих настроек инспекций нет. Зато есть код ревью, негласные соглашения по способам решении некоторых задач и голова на плечах :)
            Ну и есть CodeSniffer, для которого есть набор правил PSR2 + несколько своих кастомных, который висит на receive-hook'е git'а на удаленном сервере и не дает пушить.
            У каждого разработчика свое видение мира, строгих разделений между добром и злом нет, как и времени. Когда-нибудь появится время и что-то получится «стандартизировать» и описать какие-то правила для тулзов, которые тоже можно будет повесить на хуки и, тем самым, избегать некоторых вещей еще до того, как код уйдет на ревью.
            А пока как есть, такие дела :}
            • 0
              Когда начнёте формализоать, придётся повоевать.

              Мы просто решили: вот плагин, смотрите на что жалуется, чините и обучайтесь. Код должен быть «зелёным».

              Если есть жалобы от анализатора, сразу отправляем на доработку. НО легаси код вычищают архитекторы, попутно оставляя todo и deprecated.

              На гит переезжаем по-проектно и с ним гораздо комфортнее организовывать хуки — в целом у нас похожий подход.
  • 0
    В большинстве анализаторов удручает, что определённые правила нельзя менять для некоторого куска кода.
    Как в jslint/jshint, например. Для какого-нибудь блока можно указать комментарий, отключающий какую-то проверку.
    Типа, «да, я понял твоё указание, но именно здесь так надо.»
    • 0
      В таких случаях можно подавить ложные срабатывания на уровне выражения или всего файла. Здесь есть информацияо том, как это делается.

Только зарегистрированные пользователи могут оставлять комментарии. Войдите, пожалуйста.