Статичний аналіз 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 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 вже використовує цей оператор, а даний фрагмент коду ще просто не помітили.

Тип проблеми — 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 ситуація з інструментарієм для статичного аналізу за останні пару років помітно покращилася і, схоже, що буде поліпшуватися і далі.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.