Применяем делегирование совместно с наследованием для организации контроллеров действий

[KAndy]

Практически все методы из Common_Controller класса стоит вынести в оддельные классы, так как они нарушают принцип единственной ответственности (SOLID)

[asumin]

принцип хороший, но не слишком ли это радикально целый класс ради одного метода?

[aidme]

Ради метода в 70 строк в контроллере — нет.

[asumin]

По такому принципу?

Class Common_EditActionHelper implements Common_IActionHelper
{
       protected $controller = null;
       
       public function __construct( Zend_Controller_Action $controller ) {
           $this->controller = $controller; 
       }
     
       public function execute()  {
            // логика из _editActionHelper()
       }
}

Class Common_Controller extends Zend_Controller_Action
{
       public function executeActionHelper( $action ) {
             $class = 'Common_' . $action . 'ActionHelper'; // опустим валидацию $action для наглядности
             $helper = new $class( $this );
             $helper->execute();  
       }

}

Class VideoController extends Common_Controller 
{
       public function editAction() {
           $this->executeActionHelper( 'edit' );
       }
}

[SowingSadness]

Всё плохо.
Начнем с мелочей и дальше к крупному:
— зачем использовать префикс подчеркивания если язык поддерживает инкапсуляцию в части protected/private/public
— вызов в методе наследнике лишь метод родителя, признак плохой архитектуры
— protected нужно использовать для делегирования наследнику возможность вызова, а не для того что бы наследник переопределял работу родительского уровня. То что вам нужно, это задание абстрактного метода getModelClass, который будет возвращать имя класса (Вы покажете коллегам, что ваш контроллер завязан на определенную модель и её нужно указать обязательно)
— повсеместные статические вызовы — ужас (никогда не работал с ZF1)
— работа с глобальными массивами без обертки — плохо
— слишком много ответственности у контроллера, не понятно как с ним работать без документации. Нужно больше интерфейсов, что бы код говорил сам за себя

[Aliance]

Поясните, про префикс подчеркивания, что здесь с вашей точки зрения не так? Символ подчеркивания для закрытых полей и методов — это требования zf coding standarts.

[SowingSadness]

Хочу предложить своё видение того как всё должен работать Controller.
Что бы любой программист легко ориентировался нужно показать ему, что можно сделать только так и оставить лишь ту свободу, которую вы заложили.
Класс Controller должен получать объекты request, server и security.
Ядро должно требовать от action возврата объекта класса View
Connection к БД должен быть передан через объект сервера.
Всякие FormBuilder должны работать с request, model и возвращать View.

Соответственно больше ничем Controller нагружать не надо.

[rdaemon]

Не в основную тему поста, но так, предостережение. Если Вы блокируете в основной ветке, а снимаете блокировку только под некоторыми if'ами — можете получить проблемы.

[asumin]

это фргагменты класса, в реале есть вариант «выйти без сохранения» со снятием блокировки.

[aidme]

Все контроллеры наследники все равно получили методы-хелперы и свойства, которые нужны не всем.
В чем суть делегирования здесь? Чем данный вариант лучше способа через наследование? Т.е. если сделать абстрактный класс с нужными Action, например Model_Controller, унаследованный от Common_Controller, и уже там все это описать, ну а контроллерам которым требуются данное поведение, унаследоваться от Model_Controller?

[asumin]

В этом случае не нужные экшены будут доступны через веб, т.е. вы добавили новый контроллер Test — смотрите в него он пустой, а на самом деле, через веб доступны и /test/edit/, /test/add/ и т.д. добавили в базовый контроллер еще одни общий экшн и он сразу появился во всех контроллерах, даже в тех, где он не нужен.

[aidme]

Данные экшены будут доступны только у тех контроллеров, которые унаследованы от Model_Controller.
В базовый контроллер нужно добавлять только то, что реально нужно во всех контроллерах.

[asumin]

а можно получить гибкость

[aidme]

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

[rdaemon]

Можно сделать систему «сигналов» — в базовом

class Model_Controller 
{
...
    public function editAction()
    {
          if ($this->_hasEditAction()) {
            $this->_editActionHelper();
          } else {
            throw new Zend_Controller_Action_Exception('страница не найдена' , 404); 
         } 
    }

    protected function _hasEditAction()
    {
        return true;
    }
...
}

а в наследниках при необходимости переопределять _hasEditAction() для запрета или сам editAction() для совсем специфического поведения. Количество дублирования кода должно сильно уменьшиться.

[aidme]

del

[Aliance]

При таком подходе мы получаем т.н. «Толстый контроллер», когда там должна быть только бизнес-логика.

[asumin]

это да, но где тогда размещать эту логику? в модели тоже не правильно — она не должна знать ни о формах, ни о реквестах, получается отдельный класс как тут habrahabr.ru/post/213971/#comment_7362351

[aidme]

Не рассматривайте слой модели как один класс Active Record, моделью может быть и совокупность классов.

[asumin]

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

[aidme]

Таким классом и выступает контроллер. Берет реквест и говорит что нужно делать модели и с чем. А вот как это делать — решает уже модель.

[VolCh]

Э-э… Ничего что бизнес-логика должна быть в модели, а в контроллере логика приложения?

[nskforward]

$form->redirect->setValue( $_SERVER['HTTP_REFERER']  );

Место уязвимо к атаке. Фильтруем всё, что пришло от клиента.