рекомендации по архитектуре

[alexgivi]

нашел хорошую статью
https://habr.com/ru/post/482154/

из нее можно сформулировать несколько рекомендаций:

1. все классы могут содержать либо final public методы, либо abstract protected (для наследования), либо private.
2. если класс рассчитан на наследование - делать его абстрактным. если нет - делать его финальным.
3. не использовать наследование, кроме тех случаев, когда это надо. использовать композицию и паттерн "декоратор".

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

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

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

[index0h]

Хорошая мысль, на выходных посмотрю

[index0h]

Почитал статью, доводы вполне хорошие. Но есть несколько "но":

все классы могут содержать либо final public методы, либо abstract protected (для наследования), либо private.

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

Максимум, что могу редложить - это добавить пояснение в пункты 3.4 и 3.7, что:

• public можно переопределять в наследнике, но не рекомендуется и по умолчанию его нужно считать final.
• protected можно переопределять в наследнике, так же protected можно использовать для unit тестирования.
• final не стоит использовать явно, так как усложняет тестирование.

если класс рассчитан на наследование - делать его абстрактным. если нет - делать его финальным.

Увы, тут очень легко отстрелить себе ногу. Я бы не рекомендовал подобное. Например для сложных кейсов что бы покрыть метод тестами иногда лучше изменить модификатор приватного метода на protected. Я понимаю, что это костыль. Но юзать штуки типа mockery/softmocks для тестирования по сути сгенерированного кода - по моему костыль еще больший.

не использовать наследование, кроме тех случаев, когда это надо. использовать композицию и паттерн "декоратор".

GRASP как бы уже включает это

[alexgivi]

по поводу тестирования - есть 2 подхода: stubs (classical TDD) и mocks (mockist TDD) подробнее. т.е. можно тестировать с поведенческой т.зр, а можно с т.зр. данных. ИМХО подгонять архитектуру под тесты не стоит. лучше подумать как протестировать имеющуюся архитектуру. например вводить интерфейс для класса с единственной целью - тестирования, это ошибочный подход. возможно придется прибегнуть к магическим инструментам, благо в php возможностей для этого масса. думаю что скоро появятся инструменты, которые позволят решить эти задачи, если их еще нет. и вообще, создание системы обусловлено необходимостью иметь некий функционал, тесты тут это побочный продукт. а у вас получается тесты важнее функционала выходят. TDD, когда сначала тесты, потом уже код - ИМХО извращенский подход к разработке. но это конечно все дело вкуса... я предпочитаю писать функционал, а уже потом покрывать тестами наиболее важные и сложные классы и методы, содержащие логику. а шаблонный код, инфраструктурный (вроде методов контроллеров), или геттеры и сеттеры структур имхо вообще покрывать тестами смысла нет (напрямую по крайней мере). достаточно проверить алгоритмы через данные на входе и выходе. использовать функциональные тесты, acceptance тесты.

[EvgeniiR]

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

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

[index0h]

@alexgivi

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

Уже есть, soft-mocks. Увы, это то, что не должно существовать. Подобные инструменты работают за счет переписывания тестируемого кода. Как результат вы тестируете не свой код, а какой-то там сгенерированный в надежде на то, что его поведение совпадет с тестируемым.

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

Для меня тесты - это некий слепок функциональности моего кода. Если я что-то меняю по хорошему тесты должны упасть (иначе я написал хреновые тесты).
Я не агитирую за чистое TDD, сначала код, потом тесты, это намного проще. Но вот пушиться в репозиторий они должны сразу, так как "потом" не наступит.

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

А вот это уже не корректное утверждение. Вы можете считать что-то "важным" только на данный момент, пройдет пол года и важной может оказаться косвенная функциональность вашего кода.

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

Стоит, вот вам пример: я поправил один из контроллеров, не покрытых тестами, контроллер работает с внешним API. Мне потребовался день на разборки и тестирование, точно ли я ничего не сломал (в душе я до сих пор не уверен). Тесты сэкономили бы это день.

@EvgeniiR

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

Ну что вы, сложный код - это далеко не всегда плохой код. Весь вопрос куда вы выносите эту сложность, либо на уровень метода, либо на уровень взаимодействия методов, либо на уровень взаимодействия объектов, либо на уровень взаимодействия сервисов, либо ...
Я тут не говорил про "не получается" протестировать, я имел ввиду сложности тестирования, например описание полного окружения для конкретного кейса.

Вот вам пример, у меня с коллегой на днях был небольшой спор по теме "сложный ли это код":

// ...
public function process(array $events): void
{
    foreach ($events as $event) {
        foreach ($this->processors as $processor) {
            try {
                $processor->process($event);
            } catch (\Throwable $exception) {
                // logging
            }
        }
    }
}

Лично я не считаю этот код сложным, он же утверждал, что как минимум try-catch необходимо вынести в приватный метод, что бы метод было легко читать.

[peter-gribanov]

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

А зачем мокать реализацию? Мокайте интерфейс, а реализацию запихните в сервис который будет реализовывать этот интерфейс (SOLID). Реализацию можно отдельно протестировать. Так тесты будут проще и короче, а сложность кода меньше. Сверху можно покрыть интеграционными тестами в которых PHPUnit уже не нужен.

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

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

[alexgivi]

@index0h

Стоит, вот вам пример: я поправил один из контроллеров, не покрытых тестами, контроллер работает с внешним API

пример неправильной архитектуры. контроллер не должен взаимодействовать с внешним API. это должен делать соответствующий сервис, который должен быть покрыт тестами.

есть правда такой подход - "толстый контроллер", когда в контроллерах куча всякой логики, куча private методов, и т д. ИМХО это неверный путь. вообще, контроллер, это в каком-то смысле антипаттерн, потому что нарушает принцип единственности ответственности... скорее следует делать Action классы под каждый конкретный метод. просто некоторые фреймворки завязаны на контроллеры как основную сущность, поэтому без них не всегда получается. бизнес-логики в контроллере быть не должно, хотя бы из тех соображений, что одна и та же логика может быть вызвана из обработчика веб-запроса, из queued-задачи, из консольной команды..

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

вообще архитектура это область очень объемная, абстрактная, и дискуссионная. в этой задаче я пытался найти несколько конкретных, выраженных в коде, "маркеров" плохой / хорошей архитектуры, и сформировать соответствующие требования. например по поводу abstract/final классов, методов.

[EvgeniiR]

@alexgivi

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

И эта история не про DDD, а про архитектуру и связность.

скорее следует делать Action классы под каждый конкретный метод.

Принципиально ничего не поменяется, следование SOLID ради SOLID бесполезно.

Как прослойку между бизнес-логикой и инфраструктурой в виде фреймворка...

Отличие фреймворка от остальной инфраструктуры в том, что это фреймворк ваш код вызывает, а не вы вызываете код фреймворка, если что.

@index0h
Как ваш код связан с тестами свойств я не понял.

[index0h]

@peter-gribanov

А зачем мокать реализацию? Мокайте интерфейс, а реализацию запихните в сервис который будет реализовывать этот интерфейс (SOLID).

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

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

Бывают нюансы. Опять же сложность никуда не денется, вы ее просто вынесете на уровень выше. Я не спорю, для многих ситуаций это вполне годный кейс, но не для всех.

@alexgivi

пример неправильной архитектуры. контроллер не должен взаимодействовать с внешним API. это должен делать соответствующий сервис, который должен быть покрыт тестами.

Ок, контроллер на вход получает некий скоп данных, проверят их и передает в http клиент, ждет ответа и преобразовывает ответ для вывода. Что мне выводить в сервис?

@EvgeniiR

Как ваш код связан с тестами свойств я не понял.

Я про тест свойств ничего не говорил.

[alexgivi]

@index0h

Ок, контроллер на вход получает некий скоп данных, проверят их и передает в http клиент, ждет ответа и преобразовывает ответ для вывода. Что мне выводить в сервис?

если задача контроллера это просто передача запроса во внешний сервис, и единственное, что делается - это валидация, то тестирование валидации должно давать гарантию отсутствия ошибок в вашем сервисе, а остальное уже на совести внешнего сервиса. если бы я делал данный функционал, то у меня было бы минимум 3 класса на это: 1) класс входных данных для сервиса. 2) класс данных из сервиса 3) класс, взаимодействующий с этим сервисом. метод, который аргументом принимает объект входных данных, а возвращает объект выходных данных. в теле осуществляет вызов метода, и возможно валидацию вопроса и ответа. эти 3 класса вне контроллера, отвечают за свои частные задачи, и их можно покрыть тестами без проблем. а в контроллере логики быть не должно. вот у меня как-то так выглядел бы метод контроллера:

class SomeContorller
{
  public function someExternalCall(Request $request): array
  {
     $dataToService = SomeDataToService::createFromArray($request->getBody());
     $service = new SomeExternalServiceInteractor();
     $dataFromService = $service->someExternalMethod($dataToService);

     return $dataFromService->asArray();
  }
}

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

@EvgeniiR говоря про инфраструктуру, я имел ввиду, что создавая классы-наследники классов фреймворка (веб контроллеры, консольные команды, обработчики задач в очереди) - они не должны содержать бизнес-логику. бизнес-логика должна быть вынесена в отдельные классы,
а про DDD я имел ввиду - что раскладывать бизнес логику не по каталогам вида controllers, events, models, и т д, а по смыслу. т.е. классы, связанные общей функциональностью следует располагать рядом, а не в разных местах проекта. но это уже конечно вообще отдельная история...

[EvgeniiR]

@alexgivi
Предпочитаю использовать фреймворки которые не требуют такой неприятной штуки как наследование от вендорных классов.

А в целом всё так, только DDD тут не существеннен, и я не оч люблю баззвордами разбрасываться.

[index0h]

@alexgivi Ок, двайте внимательно посмотрим на ваш контроллер.

1. SomeDataToService::createFromArray я предполагаю, что api методы отличаются друг от друга по входящим параметрам в абсолютно большинстве случаев. Переиспользование SomeDataToService вполне возможно, но это будет происходить крайне редко. Вам правда нужен это сервис?
2. Изза статики ваш SomeContorller не может существовать без SomeDataToService::createFromArray, это не заменяемая, жесткая, не явная зависимость. По хорошему лучше использовать DI.
3. return $dataFromService->asArray вот тут вы возвращаете данные на прямую из сервиса, что далеко не всегда правильно. Данные для вывода подчинаются правилам вашего api, а вот сервис - далеко не обязательно.
4. По хорошему тут бы очень не помешал try-catch. Да, можно надейться на глобальный перехватчик вашего фреймворка, но он не всегда применим.

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

Не стоит воспринимать тесты как способ только проверки важного функционала, это ложное утверждение. Тесты - это некий слепок работоспособности вашего кода. По хорошему вы что-то поменяли - тесты должны упасть.
Все дело в том, что важность функциональности очень относительна и вы не сможете ее прогнозировать на лонг-ране, вполне нормальная ситуация, когда действительно важным для бизнеса становится косвенная функциональность того, или иного класса. Это не плохо, ни хорошо это просто есть.
Буквально на днях я столкнулся вот как раз с подобной ситуацией, не тривиальный "не важный" функционал, не покрытый тестами вдруг стал важным. Вместо того, что бы на основании упавших тестов я смог проверить, где ошибся, потребовалось всего пару дней на сбор информации, как этот "не важный" функционал должен был работать ранее. Он был не важным полтора года.

и чего тут можно сломать, что тут можно поменять, чтобы сломать - не понятно.

Буквально все что угодно, я добавлю сюда $dataToService[] = ..., а вы узнаете об этом только на продакшне. Да-да, ревью не пропустит, да-да, qa все найдут. Но согласитесь, это не всегда так, к сожалению далеко не всегда

[alexgivi]

@index0h
ок, расшифрую то, что я имел ввиду.

// вот вам класс фабрики, раз фабричный статический  метод - не кошерно.
class SomeMethodIncomingDataFactory
{
  /**
   * @throws InvalidArgumentException
   */
  public function createFromArray(array $data): SomeMethodIncomingData
  {
   ...
  }
}

class SomeExternalServiceInteractor
{
  /**
   * @throws ExternalMethodLogicException
   * @throws ExternalMethodErrorException
   */
  public function someExternalMethod(SomeMethodIncomingData $data): SomeMethodResult
  {
    ...
  }
}

class SomeContorller
{
  public function someExternalCall(Request $request): array
  {
     try {
       $factory = new SomeMethodIncomingDataFactory();
       $someMethodIncomingData = $factory->createFromArray($request->getBody());
       $service = new SomeExternalServiceInteractor();
       $result = $service->someExternalMethod($dataToService);

       return $dataFromService->asArray();
    } catch (InvalidArgumentException $e) {
      throw new ApiException($e->getMessage, ApiException::INVALID_INPUT_DATA_CODE, $e);
    } catch (ExternalMethodLogicException$e) {
      throw new ApiException($e->getMessage, ApiException::SOME_API_LOGIC_EXCEPTION_CODE, $e);
    } catch (ExternalMethodErrorException$e) {
      throw new ApiException($e->getMessage, ApiException::SOME_API_ERROR_CODE, $e);
    }
  }
}

представленный здесь сервис - уровня бизнес логики API этого приложения. для взаимодействия с внешним API может быть использован другой класс. он будет вызван внутри SomeExternalServiceInteractor::someExternalMethod()
вообще, можно было бы вынести обработку ошибок внутрь этого метода тоже, чтобы не забивать контроллер логикой.
Попытки передать массив вместо объекта вызовут 1) варнинги IDE при написании. 2) runtime error сразу при тестировании. закрыть на это глаза будет трудно.

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

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

[EvgeniiR]

Чёт не особо чего поменялось. Дёргать фабрики в контроллере - не кошерно, работа с реквестом должна быть вынесена в аргумент-резолверы. Полотно из исключений нужно переделать на респонсы.
Инициализировать сервисы контроллер не должен, для этого DI есть, или вообще выпилить сервисы в пользу контроллера, когда в нём не будет работы с реквестом.

[alexgivi]

с использованием магических инструментов фреймворка можно многое написать лаконично и кратко. правда как оно работает человек, не работавший с данным фреймворком, поймет только после глубокого изучения документации. хотя это конечно верно. все равно проекты в большинстве своем намертво завязаны на фреймворк, и все разговоры в стиле "написать фреймворк-независимый компонент" - больше из разряда красивых слов. если в каком-то фреймворке контроллеры не наследуются от класса фреймворка, то можно конечно класть туда бизнес-логику, ведь это получается чистый класс, который можно использовать и в других местах. но в laravel, yii2 такого нет, даже в symfony хотя и можно писать чистые контроллеры, но "для удобства" существуют базовые классы. вместо new SomeClass писать $this->getContainer()->get(SomeClass::class) - принципиальной разницы не вижу. только тут возникает дополнительная зависимость. в общем спорить можно... только зачем, вот в чем вопрос - все равно подходы разные, единого универсального рецепта нет и пока не предвидится... если у вас есть какие-то авторитетные источники, на которых основаны ваши взгляды (что-то вроде книг Боба Мартина) - то welcome, делитесь, будет очень полезно и интересно ознакомиться. но вроде пока такой литературы нет. обычно документация к фреймворку является таки руководством по архитектуре.