Yii Framework

Очищаю контроллеры от лишнего кода, выношу всё по максимуму в сервисы.
Встал вопрос, можно ли выносить HttpException в сервис?
Допустимо ли это по хорошей архитектуре, или это должно привести к проблемам?

Вот два варианта.

1. В контроллере только вызов сервиса. Исключение будет брошено в сервисе.

Код: Выделить всё

class ArticleController extends Controller
{
    public function actionView($id)
    {
        $article = ArticleFinder::findPublishedOrDie($id);
        return $this->render('view', ['model' => $article]);
    }
}

class ArticleFinder
{
    public static function findPublishedOrDie($id)
    {
        $article = Article::find()->where(['published' => true]);
        if (empty($article)) {
            throw new HttpNotFoundException;
        }
        return $article;
    }
}

2. В контроллере вызов сервиса, проверка и исключение. В сервисе только выборка.

Код: Выделить всё

class ArticleController extends Controller
{
    public function actionView($id)
    {
        $article = ArticleFinder::findPublished($id);
        if (empty($article)) {
            throw new HttpNotFoundException;
        }
        return $this->render('view', ['model' => $article]);
    }
}

class ArticleFinder
{
    public static function findPublished($id)
    {
        return Article::find()->where(['published' => true]);
    }
}

Вариант 1. Плюс - код контроллера проще. Минус - сервис завязан на HttpException, возможно это вообще архитектурно неправильно.
Вариант 2. Плюс - красивый сервис, не подкопаться. Минус - контроллер стал толще.

Что лучше? Кто как решает эту задачу? Прошу мнений.

Второй вариант.

А так сервис может кинуть свой NotFoundException. Не Http.

http - это уровень представления приложением данных, а сервис - это о работе с данными, и он не знает в каком виде эти данные будут представлены - http, stdout, tcp. Поэтому два разных слоя не смешиваем.

ElisDN писал(а): ↑2017.04.23, 21:07 А так сервис может кинуть свой NotFoundException. Не Http.

Задам смежный вопросик. Вот например сервис бросает какой-то LogicException, но с разными сообщениями. Как в Yii2 бросить эксепшен с человеческим текстом? Приведу, пример, как я это вижу, но может поправите.

Код: Выделить всё

class ArticleFinder
{
    /**
      * Article not found
      */
    const CODE_NOT_FOUND = 1;

    public static function findPublishedOrDie($id)
    {
        throw new DomainException("Article not found", self::CODE_NOT_FOUND);
    }
}

class ArticleController extends Controller
{
    public function actionView($id)
    {
        try {
            $article = ArticleFinder::findPublishedOrDie($id);
        } catch (\DomainException $e) {
            if ($e->getCode() === ArticleFinder::CODE_NOT_FOUND) {
                // нормально? :)
                throw new HttpException(404, "Статья не найдена.");
            }
        }
        return $this->render('view', ['model' => $article]);
    }
}

Или это не приемлемо и на каждый случай свое исключение добавлять? По аналогии с наследниками yii\web\HttpException.
Или создать эксепшен и прописать там константы кодов? (последнее мне кажется будет не удобным и дичью попахивает

)

SiZE писал(а): ↑2017.04.24, 09:41 Как в Yii2 бросить эксепшен с человеческим текстом?

Если тексты в оригинальных эксепшенах на английском, то можно легко преобразовывать через переводы:

Код: Выделить всё

try {
    $this->service->...;
} catch (\DomainException $e) {
    throw new BadRequestHttpException(Yii::t('exceptions', $e->getMessage()), 0, $e);
}

А для поиска можно либо делать orDie :

Код: Выделить всё

public function findModel($id)
{
    try {
        return ArticleFinder::findPublished($id);
    } catch (NotFoundException $e) {
        throw new NotFoundHttpException(Yii::t('exceptions', $e->getMessage()));
    }
}

либо не делать:

Код: Выделить всё

public function findModel($id)
{
    if (!$model = ArticleFinder::findPublished($id)) {
        throw new NotFoundHttpException('Статья не найдена');
    }
    return $model;
}

Спасибо за ответы.

Придумал ещё такой вариант, с добавлением дополнительного класса на обслуживание контроллеров.
Плюсы - и контроллер освободил, и файндер не испортил. Небольшой минус - классов больше стало.

Код: Выделить всё

class ArticleController extends Controller
{
    public function actionView($id)
    {
        $article = ArticleHttpService::findPublishedOrDie($id);
        return $this->render('view', ['model' => $article]);
    }
}

class ArticleFinder
{
    public static function findPublished($id)
    {
        return Article::find()->where(['published' => true]);
    }
}

class ArticleHttpService
{
    public static function findPublishedOrDie($id)
    {
        $article = ArticleFinder::findPublished($id);
        if (empty($article)) {
            throw new HttpNotFoundException;
        }
        return $article;
    }
}

Nex-Otaku писал(а): ↑2017.04.25, 09:43Придумал ещё такой вариант, с добавлением дополнительного класса на обслуживание контроллеров.

ПМСМ оверхед

SiZE писал(а): ↑2017.04.25, 14:04
Nex-Otaku писал(а): ↑2017.04.25, 09:43Придумал ещё такой вариант, с добавлением дополнительного класса на обслуживание контроллеров.
ПМСМ оверхед

бред вообще, а не подход

Аргументы?

Nex-Otaku писал(а): ↑2017.04.25, 19:51 Аргументы?

http://www.yiiframework.ru/forum/viewto ... 14#p215614

Самый последний вариант тоже что и первый.

Nex-Otaku писал(а): ↑2017.04.25, 19:51 Аргументы?

какие еще аргументы? создал сервис вокруг другого сервиса, чтобы обернуть исключение? и хочешь теперь сказать, что второй сервис - это сервис http-слоя?

zelenin писал(а): ↑2017.04.25, 22:59
Nex-Otaku писал(а): ↑2017.04.25, 19:51 Аргументы?
какие еще аргументы? создал сервис вокруг другого сервиса, чтобы обернуть исключение? и хочешь теперь сказать, что второй сервис - это сервис http-слоя?

Мне это отдаленно напомнило перехват (декоратор) https://smarly.net/dependency-injection ... terception Только статический).
Избавьтесь от статики. Она не для этого нужна. То, в каком виде вы ее используете - зло нетестируемое.

zelenin писал(а): ↑2017.04.25, 22:59 создал сервис вокруг другого сервиса, чтобы обернуть исключение? и хочешь теперь сказать, что второй сервис - это сервис http-слоя?

Да. А что не так? Конкретнее? Оценка "всё неправильно" это хорошо, но хотелось бы ещё услышать, почему неправильно.

Nex-Otaku писал(а): ↑2017.04.27, 22:44 Да. А что не так? Конкретнее? Оценка "всё неправильно" это хорошо, но хотелось бы ещё услышать, почему неправильно.

Я же дал ссылку даже на первое сообщение Зеленина, где он все написал. Что ты еще хочешь услышать? Ты принципиально это игнорируешь?

SiZE, там написано "это разные слои, поэтому их не смешиваем". А мне хочется знать, почему. Почему не сделать так? К каким проблемам это может привести? Пока что слышу только "так нельзя, потому что неправильно". Но для меня это звучит неубедительно. Убедительно будет пример, в котором вылезает какой-то косяк при таком подходе.

Я же не утверждаю, что вы не правы. Я не спорю. Просто хочу разобраться и сам понять, а не слепо положиться на чей-то авторитет. Рано или поздно я и сам разберусь, но с вашей помощью это будет быстрее. Поэтому и спрашиваю.

Суть задачи - я просто хочу максимально облегчить контроллер. Мне не нравится, что на одно действие нужно писать четыре строчки кода, я хотел бы уместить его в одну строку. Чтобы и читаемо, и красиво, и архитектуру при этом не испоганить.

Вариант с выбросом не-HTTP исключения в сервисе, конечно хороший, но получается, что нужно всё равно где-то такие исключения отдельно ловить и обрабатывать, в ErrorController или ещё где-то. Мне это не нравится, потому что нужно всё время помнить, что оно где-то там в третьем месте обработается, вести там учёт всех этих специфичных исключений и т.д.

Вариант с выбросом исключения в самом контроллере мне не нравится из-за многословности. Хотелось бы сократить код.

Также придумался ещё один вариант:

Код: Выделить всё

class ArticleController extends Controller
{
    public function actionView($id)
    {
        $article = ArticleFinder::findPublished($id);
        Assert::notEmpty($article, new HttpNotFoundException);
        return $this->render('view', ['model' => $article]);
    }
}

class ArticleFinder
{
    public static function findPublished($id)
    {
        return Article::find()->where(['published' => true]);
    }
}

class Assert
{
    public static function notEmpty($value, $exception)
    {
        if (empty($value)) {
            throw $exception;
        }
    }
}

Две строки, но хотя бы уже не четыре.

Nex-Otaku писал(а): ↑2017.04.28, 09:25 Мне не нравится, что на одно действие нужно писать четыре строчки кода...

...и Вы вместо этого для каждого действия делаете целый класс?

А действие можно с четырёх до трёх строк сократить:

Код: Выделить всё

public function actionView($id)
{
    if (!$article = ArticleFinder::findPublished($id)) {
        throw new HttpNotFoundException;
    }
    return $this->render('view', ['model' => $article]);
}

ElisDN писал(а): ↑2017.04.28, 10:19
Nex-Otaku писал(а): ↑2017.04.28, 09:25 Мне не нравится, что на одно действие нужно писать четыре строчки кода...
...и Вы вместо этого для каждого действия делаете целый класс?

Не для каждого, но именно это действие буквально на каждом шагу в контроллерах. Получил - проверил - передал.

А если для передачи между слоями использовать что-то типа Aura.Payload ?

Не говорите мне таких страшных слов )

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

Кстати говоря. Assert мне тоже не понравился, поэтому я пока что предполагаю вынести проверку в приватный метод, а его в свою очередь убрать в трейт.

Код: Выделить всё

class ArticleController extends Controller
{
    use DieOnEmpty;
    
    public function actionView($id)
    {
        $article = ArticleFinder::findPublished($id);
        $this->dieOnEmpty($article);
        return $this->render('view', ['model' => $article]);
    }
}

class ArticleFinder
{
    public static function findPublished($id)
    {
        return Article::find()->where(['published' => true]);
    }
}

trait DieOnEmpty
{
    private function dieOnEmpty($value)
    {
        if (empty($value)) {
            throw new HttpNotFoundException;
        }
    }
}

Открой для себя Middleware (Action Filters в Yii).
Все что повторяется в контролерах, связанное с HTTP выноси туда:

- Empty и другая валидация $_GET параметров
- Валидация $_POST данных
- Отлов исключение уровня Applciation и конвертация в HTTP errors.

Yii Framework

Запихнуть HttpException в сервис?

Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?

Re: Запихнуть HttpException в сервис?