PrestaShop Blocklayered

Привет Хабр! Я понимаю, что история, о которой я хочу рассказать совсем обычная. У каждого программиста, работающего с Open Source, таких случаев до десяти на дню. Но я все равно решил о ней написать. Кому-то она реально поможет, а кому-то может просто улучшит настроение, что тоже неплохо.

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

Немного про PrestaShop


Началось все с того, что начальство поставило задачу сделать Интернет-магазин. Выбор был сделан в пользу PrestaShop 1.6 по следующим причинам:

  • Написано на PHP
  • Адаптивный дизайн прямо из коробки
  • Неплохо выглядит со стандартной темой (в том числе и на мобильных устройствах).
  • Хорошо справилась с более 50 000 загруженных товаров
  • Из коробки присутствует удобный и хорошо выглядящий блок фильтров (на языке PrestaShop он называется блок многоуровневой навигации)

С последним пунктом через некоторое время и возник вопрос, ставший предметом этой статьи.

В чем проявляется глюк


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

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

Здесь и закралось неадекватное поведение. Если Вы отметили одно свойство, то все работает и новые количества появляются. Если вы отметили два и более свойств, то количество напротив позиций остальных фильтров сбрасывается до начальных (как будто ни одна позиция не выбрана).

Здесь нужно отметить, что в PrestaShop есть понятия атрибутов и понятия свойств товара. Атрибуты (attributes) – это характеристики товара, которые участвуют в формировании разных версий одного и того же товара (например размер обуви для одной конкретной модели обуви). Свойства (features) — это характеристики, общие для всех вариантов товара. В формировании вариантов товара они не участвуют, а просто информируют пользователя о потребительских свойствах.

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

Первичные предположения


Стало ясно, что:

  • Это глюк (так как с одной позицией все работает, то есть логика пересчета в код заложена)
  • Этот глюк расположен в модуле многоуровневой навигации (blocklayered)
  • Этот глюк скорее всего связан с неправильным построением условия SQL- запроса (объяснить я не могу, это больше на интуитивном уровне).

Поиски в Интернете ничего не дали, поэтому стоял выбор:

  • Оставить все как есть, согласившись, что это особенность функционирования магазина.
  • Залезть с головой в код и, проявив энтузиазм, найти причину глюка (и исправить).

Я принял второе решение. Энтузиазм убавился, когда я открыл файл blocklayered.php. Он содержал более 3,5 тысяч строк кода из которого 70% — многоэтажные SQL-запросы. Задача стала походить на поиски иголки в стоге сена. Поначалу я испугался, и даже нехорошо подумал о создателях PrestaShop. Но потом прикинул, как бы я стал программировать непростую логику работы подобного модуля и немного поуспокоился. Задача действительно сложная и, скорее всего, сложность кода вызвана объективными причинами. Но все равно, при работе с модулем не оставляла мысль, что можно сделать все это как-то покрасивее.

Инструменты и приемы


При решении проблемы будем использовать следующие инструменты:

WinSCP — надежный FTP-клиент с множеством функций. Ни разу не подводил даже на больших количествах файлов и объемах. Все функции доступны в том числе и из командной строки, что делает его полезным при написании скриптов.

UwAmp — простая в установке и настройке WAMP-сборка. Будем ее использовать для локального запуска исследуемого кода.

Notepad++ — отличный редактор для реверс-инжиниринга. Работа в разных кодировках и с разными концами строк. Хорошая подсветка синтаксиса. Открытие больших файлов. Поиск строк в том числе и по файлам в каталогах. Работает очень надежно.

HeidiSQL — GUI для MySQL. Бесплатный графический инструмент для баз данных. Иногда глючит, но в целом работать очень удобно. Используем его для изучения содержимого базы данных при анализе кода.

Основными приемами будет дамп переменных и поиск по исходному коду имен функций и кусков кода. Так как интересующие нас события происходят в том числе и в ajax-запросах дамп переменных делаем в файл. Для этого там где нужно вставляем следующий код:

$f=fopen('headfire.txt','a+');
fwrite($f,$very_important_variable);
fwrite($f, PHP_EOL);
fclose($f);

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

Начало анализа


Примерно половина кода отвечает за BackOffice. Этот код отсеиваем сразу и стараемся туда не заглядывать.

Раскрутку проблемы начинаем с поиска tpl-файла, ответственного за вывод наших фильтров на страницу. Искать долго не пришлось. Tpl-файл лежит в корне модуля и называется blocklayered.tpl. Заглянув в него убеждаемся, что в нем есть строчка вывода количества, которое у нас глючит.

<a href="{$value.link}" data-rel="{$value.rel}">{$value.name|escape:html:'UTF-8'}{if $layered_show_qties}<span> ({$value.nbr})</span>{/if}</a>

Краем глаза замечаем, что количество выводится по условию $layered_show_qties, а само количество имеет аббревиатуру nbr. Может это пригодится, а может нет.

Следующим шагом находим место, где вызывается шаблон blocklayered.tpl. Это оказывается функция

public function generateFiltersBlock($selected_filters);

Для проверки выясняем, что она вызывается два раза – один из хука левой колонки, другой из ajax-запроса. Вроде, похоже на правду. Сама функция небольшая, но в ней есть вызов функции, которая подготавливает данные для шаблона

public function getFilterBlock($selected_filters = array())

Эта функция занимает более 800 строк. В ней куча SQL-запросов. Скорее всего, здесь сосредоточена вся логика формирования фильтров. Что примечательно, что в модуле она вызывается 5 раз. Кажется, что слишком накладно вычислять столько запросов 5 раз подряд. Но потом замечаешь переменную

static $cache = null;

и понимаешь, что это старый добрый трюк с кэшированием в статической переменной. И еще понимаешь, что писали код отъявленные PHP-шники, которые не остановятся ни перед чем.

AND, OR и святая вода


Нужно как-то изучить работу функции. Глюк происходит в момент, когда в фильтре зажигается вторая галочка. А это сопровождается Ajax-запросом. Поэтому используем дамп переменной в файл.

С помощью крепкого кофе и танцев с бубнами вокруг газовой плиты находим место, где посылается основной запрос для каждого блока фильтра и вставляем туда отладочный код, выводящий этот запрос в файл (переменная $sql_query):



// headfire debug begin
$f=fopen('headfire.txt','a+');
fwrite($f,print_r($sql_query,true));
fwrite($f, PHP_EOL);
fclose($f);
// headfire debug end

$products = false;
if (!empty($sql_query['from']))
{
$products = Db::getInstance()->executeS($sql_query['select']."\n".$sql_query['from']."\n".$sql_query['join']."\n".$sql_query['where']."\n".$sql_query['group']);
}

Обратите внимание -$sql_query – массив. Это видно из кода, поэтому выводим в дамп с помощью print_r с флагом true.

Сразу первый же вывод в файл кричит нам о проблеме:

Array
(
    [select] => SELECT p.`id_product`, sa.`quantity`, sa.`out_of_stock` 
    [from] => 
					FROM ps_cat_restriction p
    [join] => LEFT JOIN `ps_stock_available` sa
						ON (sa.id_product = p.id_product AND sa.id_product_attribute=0  AND sa.id_shop = 1  AND sa.id_shop_group = 0 ) LEFT JOIN `ps_manufacturer` m ON (m.id_manufacturer = p.id_manufacturer) 
			INNER JOIN `ps_layered_price_index` psi ON (psi.id_product = p.id_product AND psi.id_currency = 1
			AND psi.price_min <= 3631136 AND psi.price_max >= 4618 AND psi.id_shop=1) 
    [where] => WHERE 1 AND EXISTS (SELECT * FROM ps_feature_product fp WHERE fp.id_product = p.id_product AND  fp.`id_feature_value` = 26634 OR fp.`id_feature_value` = 22096)
    [group] => 
    [second_query] => 
)

Обратите внимание на условие [where]: там написано в одну строчку AND и OR, причем OR находится между однородных условий и не выделено скобками.

fp.id_product = p.id_product AND  fp.`id_feature_value` = 26634 OR fp.`id_feature_value` = 22096

Я убежден, что всякий здравомыслящий программист, заметив, что в каком-то условии комбинируются AND и OR и при этом не расставлены скобки, сразу должен бежать за святой водой и окраплять ей монитор, винчестер, и клавиатуру, чтобы эта зараза не распространилась вокруг.

Если серьезно, то даже при беглом взгляде на условие и исходя из характера проблемы становится ясно, что ошибка именно здесь – OR забыли заключить в скобки. Осталось только найти это место и исправить. Но здесь нас тоже подстерегает маленький сюрприз.

Сюрприз на последок: динамическая диспетчеризация


Пытаемся найти место, где формируется ошибочное условие. Используем для этого отрывки из трассировочного вывода. Поиск по 'fp.`id_feature_value` наводит нас на функцию:

private static function getId_featureFilterSubQuery($filter_value, $ignore_join = false)

Это то, что нужно. Видим код, который формирует условие без скобок и заменяем его на правильный. Попутно хочу заметить на то, как вставляются OR – они вставляются всегда, а последний OR откусывается (причем варварским методом).

  foreach ($filter_value as $filter_val)
	$query_filters .= 'fp.`id_feature_value` = '.(int)$filter_val.' OR ';
   $query_filters = rtrim($query_filters, 'OR ').') ';

Считаю это некрасивым. Поэтому переписываю этот кусок кода в своем стиле. Внизу приводится исходный и исправленный код функции.

//modules/blocklayered/block blocklayered.php

private static function getId_featureFilterSubQuery($filter_value, $ignore_join = false)
{
  if (empty($filter_value))
      return array();

  $query_filters = ' AND EXISTS (SELECT * FROM '._DB_PREFIX_.'feature_product fp WHERE    fp.id_product = p.id_product AND ';
  foreach ($filter_value as $filter_val)
	$query_filters .= 'fp.`id_feature_value` = '.(int)$filter_val.' OR ';
   $query_filters = rtrim($query_filters, 'OR ').') ';

   return array('where' => $query_filters);
	}


//modules/blocklayered/block blocklayered.php

private static function getId_featureFilterSubQuery($filter_value, $ignore_join = false)
{
  if (empty($filter_value))
      return array();

  //headfire hack begin
  $query_filters = ' AND EXISTS (SELECT * FROM '._DB_PREFIX_.'feature_product fp WHERE fp.id_product = p.id_product AND ';
  $query_filters1 = '';
  foreach ($filter_value as $filter_val) {
     if ($query_filters1)  $query_filters1 .= ' OR ';
     $query_filters1 .= 'fp.`id_feature_value` = '.(int)$filter_val;
   }
   $query_filters .= '( '.$query_filters1.' )'.')';
 // headfire hack end
		
  return array('where' => $query_filters);
 }

А сюрприз заключается в том, что в модуле эта функция напрямую нигде не вызывается. Корявое имя наводит на грустные мысли о том, что оно где-то формируется программно и вызывается динамически. Так оно и есть, причем в остальных подобных местах сделаны честные case. А здесь решили поиграть с динамической диспетчеризацией.

Конец истории


Глюк исправлен. Фильтры стали работать красиво. Возможно эта ошибка уже решена в новых релизах PrestaShop. Ну а если нет и у Вас вскрылась похожая проблема – я рад если смог Вам помочь. И еще – не скупитесь на скобки, даже если порядок действий очевиден.
Поделиться с друзьями
-->

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


  1. jMas
    16.02.2017 12:01
    +2

    Пулл реквест с исправлениями в гитхаб проекта сделали?


    1. headfire
      16.02.2017 12:22
      +1

      Вы правы. Нужно попробовать сделать (если это еще не исправили).


  1. Stalker_RED
    16.02.2017 12:27
    +2

    Немного позанудствую.

    // headfire debug begin
    $f=fopen('headfire.txt','a+');
    fwrite($f,print_r($sql_query,true));
    fwrite($f, PHP_EOL);
    fclose($f);
    // headfire debug end
    

    Можно заменить на
    file_put_contents('headfire.txt', print_r($sql_query, true), FILE_APPEND);
    


    А еще круче — научиться использовать дебаггер.


    1. headfire
      16.02.2017 13:15

      За трюк с file_put_contents — спасибо. Не подозревал, что его можно использовать с флагами. Насчет дебаггера — может посоветуйте, что можно использовать на больших проектах (типа PrestaShop). Я как-то пробовал настроить связку с Notepad++, что-то все глючило и было очень неудобно.


      1. alexkunin
        16.02.2017 13:33
        +3

        На больших проектах лучше использовать IDE. Вроде PhpStorm.


      1. michael_vostrikov
        16.02.2017 16:24

        Хм, внезапно пригодилось: XDebugClient
        Плагин отладчика для Notepad++. Писал для себя 3 года назад. Написан на C#. Код ужасный конечно.


        1. headfire
          16.02.2017 17:08

          Спасибо.


    1. ilyaplot
      16.02.2017 14:55

      $logger = new Monolog\Logger();
      $logger->addDebug('SQL:', $sql_query);
      


  1. maghamed
    16.02.2017 13:45
    +2

    В статье прекрасно все — от методов дебагинга через дамп в файл, до исправлении бага в core файле магазина.
    Хотя судя по этому листингу Prestashop их код особо и не расчитан, чтобы его нормально кастомизировали и правили


    1. hector
      16.02.2017 14:31

      Правился все-таки модуль, хоть и официальный. В данном движке есть способ кастомизации модулей, что позволит обновлять модуль из админки.


  1. maghamed
    16.02.2017 13:54
    +4

    За такой код надо казнить

      foreach ($filter_value as $filter_val)
    	$query_filters .= 'fp.`id_feature_value` = '.(int)$filter_val.' OR ';
       $query_filters = rtrim($query_filters, 'OR ').') ';
    


    Вы и руками пишете такую аброкадабру, когда есть конструкция IN?

    $query_filters =  'fp.`id_feature_value` IN (' . implode(',', $filter_value) . ')'
    


    Абсолютно нечитаемо и неэффективно, также как и конструкция вроде EXISTS.


    1. headfire
      16.02.2017 14:09

      Код с rtrim не из красивых. Я об этом в статье упомянул и исправил в меру своих сил. Ваше решение с IN элегантнее, чем с OR. Мне оно в голову не пришло (хотя должно было бы). Просто я не думал о рефакторинге самого SQL — я исправлял алгоритм его формирования.


  1. sir_Maverick
    16.02.2017 14:20
    +1

    Мда, преста никогда не отличалась красотой кода… Помню, как правил основные цвета темы, раскиданные по всему файлу: кнопка здесь, ховер на кнопку там, клик еще гдето.


    1. ilyaplot
      16.02.2017 14:30

      А вы видели opencart?


      1. headfire
        16.02.2017 14:36

        В OpenCart самое интересное, что в ней модули подключаются через специальную примочку, которая на лету исправляет PHP-код ядра. Таким странным образом решается проблема расширения функциональности и кастомизации.


        1. xRay
          16.02.2017 15:56

          Это в старых версиях так было. Сейчас вроде сделали нормальную систему плагинов.


      1. sir_Maverick
        16.02.2017 14:37

        Опенкарт практически не щупал, но подозреваю, что там так же, вроде читал где-то, что преста выросла из opencart. Могу ошибаться.


  1. ilyaplot
    16.02.2017 14:29
    -1

    Я подобных задач в день не один десяток решаю. Может, сначала стоит до конца изучить азы языка, а потом писать на хабр?


  1. ilyaplot
    16.02.2017 14:35
    -1

    Очень хочется видеть в таких местах prepeared statement.
    Предложу вот такой код:

    private static function getId_featureFilterSubQuery($filter_value, $ignore_join = false)
    {
      if (empty($filter_value))
          return array();
    
      //headfire hack begin
          $query_filters = ' AND EXISTS (SELECT * FROM '._DB_PREFIX_.'feature_product fp WHERE fp.id_product = p.id_product AND ';
          $query_filters .= ' fp.`id_feature_value` in (' .implode(', ', 'intval', $filter_value)). '))';
    		
          return array('where' => $query_filters);
     }
    

    P.S. скобки не считал, мог ошибиться.


    1. ilyaplot
      16.02.2017 14:40
      -1

      Прошу прощения, array_map потерялся.

      private static function getId_featureFilterSubQuery($filter_value, $ignore_join = false)
      {
          if (empty($filter_value))
              return array();
      
          //headfire hack begin
          $query_filters = ' AND EXISTS (SELECT * FROM ' . _DB_PREFIX_ . 'feature_product fp WHERE fp.id_product = p.id_product AND ';
          $query_filters .= ' fp.`id_feature_value` in (' . implode(', ', array_map('intval', $filter_value)) . '))';
          return array('where' => $query_filters);
      }
      


    1. ilyaplot
      16.02.2017 14:53
      -2

      Можно извлечь урок: не делай релиз без тестов.