PrestaShop. О глюке в многоуровневой навигации

    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. Ну а если нет и у Вас вскрылась похожая проблема – я рад если смог Вам помочь. И еще – не скупитесь на скобки, даже если порядок действий очевиден.
    Ads
    AdBlock has stolen the banner, but banners are not teeth — they will be back

    More

    Comments 21

      +2

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

        +1
        Вы правы. Нужно попробовать сделать (если это еще не исправили).
        +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);
        


        А еще круче — научиться использовать дебаггер.
          0
          За трюк с file_put_contents — спасибо. Не подозревал, что его можно использовать с флагами. Насчет дебаггера — может посоветуйте, что можно использовать на больших проектах (типа PrestaShop). Я как-то пробовал настроить связку с Notepad++, что-то все глючило и было очень неудобно.
          • UFO just landed and posted this here
              0
              Хм, внезапно пригодилось: XDebugClient
              Плагин отладчика для Notepad++. Писал для себя 3 года назад. Написан на C#. Код ужасный конечно.
                0
                Спасибо.
              0
              $logger = new Monolog\Logger();
              $logger->addDebug('SQL:', $sql_query);
              
              +2
              В статье прекрасно все — от методов дебагинга через дамп в файл, до исправлении бага в core файле магазина.
              Хотя судя по этому листингу Prestashop их код особо и не расчитан, чтобы его нормально кастомизировали и правили
                0
                Правился все-таки модуль, хоть и официальный. В данном движке есть способ кастомизации модулей, что позволит обновлять модуль из админки.
                +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.
                  0
                  Код с rtrim не из красивых. Я об этом в статье упомянул и исправил в меру своих сил. Ваше решение с IN элегантнее, чем с OR. Мне оно в голову не пришло (хотя должно было бы). Просто я не думал о рефакторинге самого SQL — я исправлял алгоритм его формирования.
                  +1

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

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

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

                      –1
                      Я подобных задач в день не один десяток решаю. Может, сначала стоит до конца изучить азы языка, а потом писать на хабр?
                        –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
                          Прошу прощения, 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);
                          }
                          
                            –2
                            Можно извлечь урок: не делай релиз без тестов.

                          Only users with full accounts can post comments. Log in, please.