Pull to refresh

Comments 46

Спасибо за отзыв. Очень боялся негативных отзывов. После вопроса habrahabr.ru/qa/34735/ были неоднозначные ощущения.
Вместо ручного формирования ответа в экшене контроллера

$this->getResponse()
            ->setHttpResponseCode(200)
            ->setHeader('Content-type', 'application/json', true)
            ->setBody(Zend_Json::encode($result));

можно использовать JSON Action Helper и тогда код сократится до:

$this->_helper->json($result);
Промахнулся ответом :(
Не совсем согласен с Вашей реализацией сервисов и на мой взгляд она содержит следующие недостатки:
1) public static function login (array $params) — передавать все параметры в виде массива это плохая привычка, если использовать такой подход со временем окажется что в ваш сервис надо передать массив из 20-100 параметров
в Вашем случае достаточно обьявить функцию:
public static function login ($email, $password)
Согласитесь что это намного понятнее чем массив и позволяет использовать сервис без необходимости читать его код или документацию (которую кстати надо сначала написать, а потом поддерживать)
Кроме того передача параметров явно очень полезна т.к. если вдруг вам понадобиться сервис в котором более 3-х параметров вы уже задумаетесь о разделении ответственности.
2) Статические сервисы хороши только для логина в вакууме — во всех остальных случаях лучше все же создавать обьект и пользоваться нестатическими методами. Это даст гораздо большую гибкость если Вам понадобятся более сложные сервисы. Например можно будет легко декорировать сервисы дополнительным функционалом (кеширование, изменение формата данных для разных версий АПИ например). Для того чтобы облегчить создание сервис обьектов советую посмотреть на паттерн DependancyInjection Container и как он реализован в Zend2 и Symfony2 хотя я и не считаю их реализацию очень удачной (в основном изза ограничений самого пхп)
Спасибо за критику по существу.

Насчет параметров — ну скажу так, это достаточно холиварные вопросы, и каждый из вариантов содержит свои плюсы и минусы. Да, можно и так, как вы предложили, но модно и так, как реализовал я, а также есть еще 100500 способов.

Насчет статических сервисов. Для начала скажу, что они у меня не все статические. Все таки я привел «нейтральный код». Для рассматриваемой логики в сервисах, я считаю, что лучше сервисы сделать статическими.

А насчет dependency — в ближайших планах столкнуться с zf2 )
<придирчивость>
При необходимости подключить дополнительный источник данных, например, завтра мы решим использовать дополнительно NoSQL базу, или начать использовать кэш, то нам будет достаточно декорировать.
            $adapter = new Zend_Auth_Adapter_DbTable(
                    Zend_Controller_Front::getInstance()->getParam('bootstrap')->getPluginResource("db")->getDbAdapter(),
                    'user',
                    'email',
                    'password',
                    'MD5(CONCAT(?, passwordSalt,"' //MD5(пароль + уникальный хэш + общий хэш)
                    . Zend_Controller_Front::getInstance()->getParam('bootstrap')->getOption('salt') . '"))'
            );

Кроме декорирования, вам придется переписывать сам сервис. MD5 и CONCAT не валидны не только для NoSQL, но даже для других СУБД. Солить пароль, я думаю, стоит на уровне модели.
</придирчивость>
Соглашусь с вами. Там разные контроллеры есть. Если смотреть на конкретную схему реализации контроллера авторизацию — его можно полностью переписать, так как мы ушли на другую схему авторизации, где можно сделать проще.
Как говорится, руки сюда еще не дошли. Спасибо за замечание.
Если посмотреть на метод logout, то там видно, что zend_auth скорее артефакт, нежели рабочая схема.
Context switcher в статье упоминается.
Я его лично пока что не использую, так как нет нужды.
Спасибо за наводку, но есть ряд со мнений.
1) ссылка на zf2. Не уверен что этот хелпер есть и в первой ветке.
2) не всегда нужно возвращать 200 статус, опять таки, надо посмотреть на практике, как будет себя вести
3) не будет ли перекрываться setNoRender, как в случае с json_encode?
UFO just landed and posted this here
Вас архитектура не понравилась или названия классов?
Тут уж как говорится «вам шашечки или ехать»?

Насчет мапперов — я не приводил примеры абстрактного классов и более сложных моделей и мапперов, но написал, что данные можно и собирать в нужный нам объект, и в конструкторов можно инстанцировать сразу загрузку. Но только ведь тут два но:
1) статья о другом
2) в статье указаны все эти возможности и рекомендации. Вас смущает, что я привел не весь код проекта? Извините, он коммерческий.

Насчет мапперов, аозаращаемых данных в zend_db_row и тд — по моему вы очень не внимательно ознакомились со статьей.
Прошу прошения за ряд опечаток. Печатал с телефона.
UFO just landed and posted this here
Ну, вас же предупреждали, что не нужно трогать мёртвую кошку, а то распахнется. Буду критиковать, но с попытками конструктивно послать в правильном направлении.
По порядку, файловая система, в зенде давно была впиленая модульная система, которая позволяет собрать файлы в наборы модулей. которые затем можно спокойно переносить из проекта в проект framework.zend.com/manual/1.12/ru/zend.controller.modular.html
Дальше для яджакса существует понятие смениы контекста stackoverflow.com/questions/7065378/clean-ajax-controller-implementation-in-zend-framework Удивительно что ваш код не ругается на отсутствие лэйаута вызова не нашёл его отключения$this->_helper->layout()->disableLayout();
Дальше if (!is_null($result['secretKey'])) и здесь я с ужасом понял что вы пишете код с отключенными нотисами.
Дальше вы писали про тестирование, а затем лепите статик вызовы $result = Application_Service_DeviceService::login($params), жёстко хардкодите зависимость да ещё с отсутствие её мочить прии тестировании.
Дальше у вас нет своего прокси класса для Zend_Controller_Action и сервисов, из-за чего приходится писать длинные цепочки вызовов для посылки заголовков или таскания из бутстрапа ресурса db.
У молелей типа как в методе signup вы устанавливаете волшебные аттрибуты, хотя нужно было выделить отдельный метод, который был бы единой точкой расположенной в самой модели где легче регулировать эти параметры. А вот для deleteFile вы создали лишний метод не связанный с самой моделью и программист должен помнить что нужно удалять файл, а не просто саму запись, нужно было конечно оверврайтить стандартный метод delete, чтобы сохранять целостность.
И т.д. и т.п. Зендом пользовался лишь пару раз, да и то в основном с доктриной, но уж у вас проблемы более чем очевидные и вопиющие, главным признаком которых является куча кода.
Проблема не в «мертвой кошке». Я очень много PS написал на этот счет.
Также по порядку отвечу.
Про файловую систему — знаю про модульность, но так как тема называется не «как сделать свое предложение на ZF», и не «Организация модульной архитектуры», решил специально опустить эту тему. С точки зрения архитектуры модели, не важно, сделано именно в таком виде и модель находится наверху, или же собрано в модуль, и модель находится внутри модели.
Насчет смены контекста — в рамках одного контроллера у меня нет нужды (исходя из задач) менять контекст. Насчет layout — использую setNoRender. Пока нотисов не видел.
Нотисы не отключены. Приведенный код упрощен. Про зависимость — не понял. Не могли пояснить. Также не совсем понял контекст слова «мочить ее».

«Дальше у вас нет своего прокси класса для Zend_Controller_Action и сервисов, из-за чего приходится писать длинные цепочки вызовов для посылки заголовков или таскания из бутстрапа ресурса db.»
А зачем? Не могли бы на примере показать, что это даст, для чего и тд. Буду благодарен.

«У молелей типа как в методе signup вы устанавливаете волшебные аттрибуты, хотя нужно было выделить отдельный метод, который был бы единой точкой расположенной в самой модели где легче регулировать эти параметры»
Я некоторые вещи опустил, некоторые используются в абстрактных классах. Опять таки, мы сейчас говорим не о теме поста.

«А вот для deleteFile вы создали лишний метод не связанный с самой моделью и программист должен помнить что нужно удалять файл, а не просто саму запись, нужно было конечно оверврайтить стандартный метод delete, чтобы сохранять целостность.»
В каком плане? На мой взгляд, именно это и есть действие над объектом, а значит именно это и должно быть в модели. Овверрайдить метод, который в маппере? А если завтра мы начнем хранить в MongoDB? Нам надо будет все переписать?
Насчет целостности — если мы не смогли удалить файл — ну ок. На мой взгляд, из этого не надо делать транзакцию. Ну по крайней мере в контексте моего проекта.

«И т.д. и т.п. Зендом пользовался лишь пару раз, да и то в основном с доктриной, но уж у вас проблемы более чем очевидные и вопиющие, главным признаком которых является куча кода.»
А где там куча кода? Я в статье написал, что доктрина может забрать на себя пару уровней, но речь то не об этом. Вы пытаетесь выдрать контекст и найти косяки в совершенно отчужденных местах. Вы бы еще сказали, что названия плохие для классов, что строчки выравнены плохо, там нет 4 пробелов зендовских, поэтому статья — полное УГ. Ей богу.
Не смешите, у вас про модель ничего почти нету, только калька на манульный пример, ссылку на которой минусонули в Q&A habrahabr.ru/qa/34735/#answer_135223, где действительно про модель. Лучше бы мануал по фреймворку осилили, чем спорили и выгораживались.
Мануал осилил. Спасибо.

А что такое модель в вашем понимании?

В моем — несколько слоев, которые отвечают за сами данные и тд (см статью). Каждый из этих уровней я старался максимально расписать, в том числе с примерами кода.

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

Собственно, судя по этому комментарию, есть ощущение, что вы сами статью то не до конца осилили.

Опять таки, я написал, что не претендую на «идеальное решение». Я не нашел в сети такой структурированной информации и потратил некоторое количество времени на разбор и написание. Если вы не оценили — очень жаль. С радостью почитаю вашу статью.

PS Не я минусовал вышу ссылку. могу скинуть скрин :)
1. Почему сервисы статичные?
2. Зачем тестировать контроллеры, логичнее ведь тестировать сервисы?
1. Вы знаете, у меня нет однозначного ответа и 100% уверенности, что это правильно. Но на данных примерах, я не увидел «для чего нужно создавать объект сервиса». Есть методы в сервисах, связанные с контекстом. В пример они не попали, было бы лучше, если бы были видны различные подходы. Если вы считаете, что эти методы не должны быть статическими, поясните почему, и я вполне смогу с вами согласится.

2. Логичнее тестировать все — и контроллеры, и сервисы, и мапперы и модели.
Опять таки, я для примера показал, что в таком виде все достаточно легко тестируется… А цель этой статьи — все таки не «как правильно тестировать в ZF».
1. А мы помним что происходит с статичными переменными и функциями, как они работают и как они хранятся?
Если мне не изменяет память, то при компилировании проекта, под статические медоты и функции сразу выделяется пямять, в не зависимости, будите ли вы создавать объект класса или нет.

А с логической точки зрения, выделение медота login в область видимости всему проекту, особенно, когда login используется только один раз в одном месте, собственно, при залогинивании, не есть хорошо. ИМХО.
хм… Мне казалось наоборот, потребление меньшее. Ведь так получается, что в каждом месте, где мы используем, нам необходимо создать объект, и так проводится полная инициализация. А так -все же меньше. Могу ошибаться.
Также, если у меня тут нет явной зависимости между методами и переменными, но так проще изначально думать о дальнейшем рефакторе. А связывать это все вместе — мне кажется не правильная мысль. Тем более, связывание будет искусственным.

Беглое гугление дало такую ссылку www.sql.ru/forum/actualthread.aspx?tid=716873, но поищу еще.

Насчет области видимости… А что нам будет мешать создать объект класса сервиса в любом месте проекта и вызвать этот метод?
Не красиво просто это. Ну в примере с залогиниванием — точно не красиво.

Вот хороший пример (на мой взгляд):

Есть класс Humanity, который имеет 2 функции declension (Склонение существительных по числовому признаку) и human_date (Выводит дату в приблизительном удобочитаемом виде (например, «2 часа и 13 минут назад»)).

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

А вот метод залогинивания, который используется в одном месте и всего лишь один раз — не стоит, не красивое решение :)
Ну во-первых, он у меня используется не один раз ;)

Просто инстанцировать объект вместо статики и оперируя лишь аргументом — мы будем это использовать только один раз. Ну ок. А зачем, если мы будем инстанцировать только в одном месте, то создавать под это лишний объект?
        // decode from json params
        $params = Zend_Json::decode($data);

        $email = $params['email'];
        $name = $params['realName'];
        $password = $params['password'];

        $err = array();
        if (!isset($email) || !isset($name) || !isset($password) || (filter_var($email, FILTER_VALIDATE_EMAIL)==FALSE)) ...........

Для чего надо делать проверку !isset($email) || !isset($name) || !isset($password), если переменные явным образом определены несколькими строчками ранее?

Дело в том, что соответствующих элементов массива после Zend_Json::decode() может не быть, а это уже: Notice: Undefined index: email.
Мы не можем быть уверены точно, что JSON придёт именно тот, что нам нужен.
В рамках данного контекста кода, на мой взгляд проверку как раз таки надо было делать так:

$email    = isset($params['email']) ? $params['email'] : null;
$name     = isset($params['realName']) ? $params['realName'] : null;
$password = isset($params['password']) ? $params['password'] : null;

После того, как переменные определены и имеют какие-то значения, только тогда уже проверять и не isset(), а уже null !== $email.
И вообще, код в DeviceapiController::signupAction() мягко говоря шокировал меня:

if (!isset($email) || !isset($name) || !isset($password) || (filter_var($email, FILTER_VALIDATE_EMAIL)==FALSE))
        {
            if (!isset($email)) {
                $err['email'] = "Email is missing";
            }

Зачем так делать?!
Я понимаю, что это всё примера ради, но всё же надо позаботиться, что бы это было как подобается, тем более, как я понял — это копипаст с рабочего проекта.
Для чего надо делать проверку !isset($email) || !isset($name) || !isset($password), если переменные явным образом определеный несколькими строчками ранее?

Там может быть NULL. Соглашусь с тем, что, возможно, стоило бы проверять сразу params['...'], а лишь потом присваивать переменным, либо вообще не использовать доп переменные.

После того, как переменные определены и имеют какие-то значения, только тогда уже проверять и не isset(), а уже null !== $email.

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

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

А как сделали бы вы? Если вы поможете мне понять мои ошибки, это позволит мне уже сегодня начать писать еще более лучший код. А значит это было не зря и для меня. Мы все учимся. Постоянно.

Насчет копипаста — с ньюансами, но в целом ~70-80% кода не изменял.
Ну, к примеру, вот набросал в рамках вашего кода:

        $params = Zend_Json::decode($data);
        $errors = array();

        $validate = function(array $what) use ($params, $errors) {
            if (!is_array($params) || empty($params)) {
                throw new InvalidArgumentException('Invalid params has provided');
            }
            foreach ($what as $name) {
                if (!isset($params[$name])) {
                    $errors[$name] = ucfirst($name) . ' can\'t be empty!';
                } elseif ('email' == $name) {
                    $validator = new Zend_Validate_EmailAddress();
                    if (!$validator->isValid($params[$name])) {
                        $errors[$name] = current($validator->getMessages());
                    }
                }
                /*
                 * Your additional logic here
                 * .................................
                 */
            }
        };

        try {
            $validate(array('email', 'name', 'password'));
        } catch (InvalidArgumentException $e) {
            // Handle exception
        }
        
        if (empty($errors)) {
            // Success
        } else {
            // Failure
        }

А лучше сделать всё сервисом, используя factory pattern, для того что бы иметь возможность авторизовать пользователя к примеру через Facebook или Twitter, а не только учётной записью вашей локальной базы.
Имея такой подход можно с лёгкостью добавлять новые методы авторизации, написав лишь для каждого новый адаптор имплементрирующий конвенцию интерфейса, которую вы уже сами реализуете.

Тут, конечно, на вкус и цвет… Но, надо приучать себя писать scalable код, потому как никогда точно не знаешь, как может в последствии развиться проект и какие новые потребности могут появиться. Экономиться куча времени на рефакторинге и нервов, когда в конечном итоге приходиться всё переписывать. А тут, оп-оп и в дамках! :-)

Статья не плохая, развивайтесь!
Удачи вам!
Стоит отметить, что этот код будет работать, начиная с версии 5.3.
И еще тут уместнее не factory, а strategy.
На дворе 2013 год, вы ещё пишете на <=5.2?
Почему лучше strategy? Обоснуйте.
Этот уровень в моей реализации ничего не знает о способах (и местах) хранения данных.

Так зачем тогда, засунули следующий кодв модель:
/* Start describe behaivors of object
     */
    public function getDeviceByKey ($key) {
        return $this->_mapper->findByKey($key);
    }
    
    public function deleteByKey($key) {
        return $this->_mapper->deleteByCriteria('secretKey', $key);
    }


Вынесети, такого рода методы в маппер, он ведь знает и от DataAccessLevel и от Domain Model.
Так сама имплементация же и есть в маппере. А маппер уже знает о местах хранения данных.
Если у нас изменится место хранения, то метод в модели останется тем же, а имплементация в маппере изменится. Ну и определим другой маппер. Или я не верно вас понял?
Подходов к реализации патернов много, но вот подход Доктрины, намного ближе к сути. Когда Модели ни чего не знаю об окружающем мире, кроме отношений между собой.
К примеру:
 $user = new Article();
 $user->setName();
 $user->setFirstname();
 $article = new Article();
 $article->setTitle('Doctrine model');
 $article->setUser($user);
 $em->persist($user);
 $em->persist($article);
 $em->flush();

$em->remove($article);
$em->flush();

Собственно Zend 2 предлагает аналогичный подход к реализации моделей, за некоторыми исключениями.
Полностью согласен с вами в этом моменте.
Для себя на следующий проект хочу взять связку ZF2 + Doctrine попробовать.

Но у меня были изначально другие входные данные и ограничения, поэтому я старался найти эффективное решение внутри ZF1, так как стандартная реализация мне показалась неэффективной.
Очень интересное разделение модели на несколько уровней.
А не боитесь при этом получить более сложное обнаружение ошибок?
В случаи с тестированием проекта намного проще протестировать пару маленьких классов, но при появлении бага с залогиниванием, скажем пользватель не логинится, вам придется лезть в проект и копать кучу классов и искать место бага в них.
Вместо того, чтоб открыть одну модель UserTable и глянуть в ней функцию login.

И небольшая идея, как уменьшить ваш контроллер.
Используйте формы.
Понимаю что вы подумали, у меня тут API приложение, зачем мне формы?
Но формы нужны не только для их вывода, они представляют из себя хороший компонент для фильтрации и валидации данных!

К примеру, ваш код

$email = $params['email'];
$name = $params['realName'];
$password = $params['password'];

$err = array();
if (!isset($email) || !isset($name) || !isset($password) || (filter_var($email, FILTER_VALIDATE_EMAIL)==FALSE))
{
if (!isset($email)) {
$err['email'] = «Email is missing»;
}
if (!isset($name)) {
$err['name'] = «Name is missing»;
}
if (!isset($password)) {
$err['password'] = «Password are missing»;
}
if (filter_var($email, FILTER_VALIDATE_EMAIL)==FALSE) {
$err['valid_email'] = «Email is not valid»;
}
}

превратится в красивый и правильный

$loginForm = new LoginForm();
if ($loginForm->isValid($params)) {
// all cool!
} else {
$this->getResponse()
->setHttpResponseCode(400)
->setBody(Zend_Json::encode(array («invalid» => $loginForm->getErrors())));
return;
}

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

За мысль с формами — спасибо. Когда перешли на Апи, побоялся их использовать. Сказался недостаток опыта работы с ZF. Возьму на заметку.
Если НЕ хотите Form(как я, например), Можно использовать Zend_Filter_Input. То же самое, но без Рендерера.
Поддержу про тестирование — такая изначально глубокая иерархия ради иерархии только создаёт сложности в поддержке и тестировании.

Мне кажется нужно взять какие-то базовые общепризнанные слои (MVC), и делить их при появлении явной необходимости.

Да, это будет относительно трудоёмко при переделке, но ведь большой шанс что не понадобится вообще. А сейчас сложности уже есть.
А в чем сейчас сложности?

Можно вообще все в одном файле делать. Вы будете точно знать, что вдруг чего — у вас все в одном файле. Удобно же? Мне вот нет.

Также замечу, что иерархия далеко не ради иерархии.
Прошу прощение за оформление кода. Тег source почему-то не хочет работать :(
вставлю свои пять копеек.
у вашего кода есть «запах». просто отвлеченно посмотрите на это
Application_Model_Abstract_AbstractModel

array $options = null

$this->_setAdapter(Zend_Db_Table::getDefaultAdapter());


->setHttpResponseCode(400)

Вы уверены, что правильно используете эти коды? Для клиента они имеют какой-нибудь смысл?

Короче, получилось примерно как вы писали в habrahabr.ru/qa/32135/ «Тут вопрос скорее не в том, где писать запросы в базу, а как это красивее разнести :) Именно, как вы и сказали — архитектура :)». Вот эта самая «архитектура» вам пока и мешает. Можно даже сказать, что на начальных этапах архитектура — это синоним для «ненужное усложнение».
// Я сторонник подхода, когда реализуется тонкий контроллер, а весь бизнес выносится в сервисы и модели.

Полагаю, что это единственный и верный подход.

Я поступал следующим образом: бизнес логика выносится в действия (Actions) — это по сути атомарная единица в функциональном разрезе приложения. Валадиция данных производиться там же. По сути это черный ящик на вход которого подаются данные а на выходе мы получаем результат. Задача контроллера контролировать выполнение этого действия, обрабатывать ошибки и отправлять результать выполнения действия на дальнейшую обработку. В самом действии могут быть использованы различные сущности предметной области (модели). Если для хранения данных используется реляционная БД, то контроль транзакции также осуществляется контролером а не моделью.

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

class Order extends CController
{
function makeOrder()
{
// If passed data is not valid an exception is being thrown
$action = new Action\MakeOrder($this->getPostData());
// Perform our action.
$action->perfom();
// Get context of a template rendering the order form.
$tplCtx = $this->getLayout()->getTemplateById('someTplId')->getContext();
// Check errors
if ($action->hasErrors())
{
switch ($action->getErrorType())
{
case Action\MakeOrder::ERR_LOW_BALANCE:
$tplCtx->setLowBalanaceErr(true);
//…
}
}
}
// Magic method. Being called if an exception in the method ::MakeOrder happens.
function __exceptionMakeOrder()
{

}
}

Sign up to leave a comment.

Articles