Задание 0 #1

AndrewTemchishen · 2021-12-18T22:36:24Z

Первый мой проект на Symfony и решил сразу на 6 с php 8.1. Соответственно без api platform.
Идея была в том, чтобы сделать генерацию keyword для короткой ссылки на уровне id в сущности.
Потратил много времени на документацию + разные неожиданные ошибки.
В данный момент при попытке что нибудь сделать валится в фаталити. Я так предполагаю, что накосячил с аннотациями или нужно запускать в режиме no-debug

`Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 20480 bytes) in /srv/app/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php on line 728

Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 65536 bytes) in /srv/app/vendor/symfony/error-handler/DebugClassLoader.php on line 287`

КОРОТКО: На сервисы разделить не успел, валидацию не реализовал, действия по переходу по ссылке тоже... Не хватило времени и знаний по фреймворку.
Хотелось бы узнать как можно разделить на сервисы логику в этом задании, как правильно реализовать валидацию

It's isnt work because memory leak in DebugClassLoader and AnnotationDriver

YourCodeReview · 2021-12-20T09:43:45Z

На что именно хочет получить фидбэк?
В целом узнать правильно ли используется фреймворк, best practice. Узнать как можно разделить на сервисы логику в этом задании, как правильно реализовать валидацию

Вопросы ревьюерам
Интересно почему на состоянии коммита cb29411 при попытке создать ссылку или сделать миграцию происходит утечка
Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 20480 bytes) in /srv/app/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php on line 728
Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 65536 bytes) in /srv/app/vendor/symfony/error-handler/DebugClassLoader.php on line 287

lysiakin

Надеюсь ответил на заданные тобой вопросы и чуток прояснил что к чему ☺️ Ах да, касаемо фаталов, надо стягивать код и разбираться по факту. Так сказать сложно.

lysiakin · 2021-12-21T21:27:03Z

.env

+#  * .env.$APP_ENV       committed environment-specific defaults
+#  * .env.$APP_ENV.local uncommitted environment-specific overrides
+#
+# Real environment variables win over .env files.


Комментарии пакетов и сгенерированный текст желательно бы убирать.

lysiakin · 2021-12-21T21:30:27Z

.idea/UrlShortener.iml

@@ -0,0 +1,73 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<module type="WEB_MODULE" version="4">
+  <component name="NewModuleRootManager">


Понимаю, что папочку с конфигом phpstorm не заигнорил по невнимательности, но всё же 😉

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

.idea папку как правило заносят в .gitignore , однако сами JetBrain рекомендуют некоторые файлы (отвечающие за внешние папки, конфигурацию дебага и информацию о cvs) коммитить

lysiakin · 2021-12-21T21:37:42Z

Dockerfile

+ENV SYMFONY_VERSION ${SYMFONY_VERSION}
+
+# Download the Symfony skeleton and leverage Docker cache layers
+RUN composer create-project "${SKELETON} ${SYMFONY_VERSION}" . --stability=$STABILITY --prefer-dist --no-dev --no-progress --no-interaction; \


Создание приложения в Dockerfile - не лучшее тому место. Наш контейнер должен быть наиболее атомарен. Например, содержать сугубо php-fpm в себе. Как и рекомендуется самим докером - у одного контейнера - одна зона ответственности.
К тому же с каждой директивой dockerfile становится тяжеловеснее, приобретая по слою за команду. Что тоже не очень сказывается на его размере и скорости разворачивания.

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

Это действительно изменённый Dockerfile из рекомендаций symfony по работе с докером . Это нормальное решение, однако, как заметил @DaringFrogling, не атомарное, потому что код не отделён от инфраструктуры. Идеологически правильно было бы код хранить в отдельном контейнере и шарить его volume между контейнерами, но при этом, увеличивается сетевая нагрузка внутри докера, что будет добавлять неудобства при разработке (особенно касается macos)

lysiakin · 2021-12-21T21:58:47Z

migrations/Version20211216203025.php

+        // this down() migration is auto-generated, please modify it to your needs
+        $this->addSql('CREATE SCHEMA public');
+        $this->addSql('DROP SEQUENCE option_id_seq CASCADE');
+        $this->addSql('CREATE SEQUENCE link_keyword_seq INCREMENT BY 1 MINVALUE 1 START 1');


Не понимаю почему в down() создаётся сиквенс и схема.

Автогенерация, не знал, что за ней нужно следить 😓

Не понимаю почему в down() создаётся сиквенс и схема.

Потому что в up она дропается. Значит, были изменения в Entity

lysiakin · 2021-12-21T22:01:40Z

src/Command/InitOptionsCommand.php

+
+    protected function configure(): void
+    {
+        $this->setHelp('This command insert needed records to options table in db');


Фикстуры лучше наполнять с помощью DoctrineFixturesBundle. https://symfony.com/bundles/DoctrineFixturesBundle/current/index.html

То есть инициализировать конфиг в бд при помощи фикстур? Но это так же делать в команде?

Если данные обязательно должны быть то допустимо инициализировать их в миграции. Более правильный вариант - фикстуры (с ними нужно быть осторожным - неверный вызов перезатрёт имеющиеся в бд данные). Данный вариант с вызовом команды тоже возможен, но он менее очевидный (его нужно прописывать в документации к деплою). Так же допустим и вариант создания next_id во время выполнения кода, без необходимости запуска каких либо команд

lysiakin · 2021-12-21T22:28:00Z

src/Controller/JsonErrorController.php

+use Symfony\Component\HttpFoundation\JsonResponse;
+use Throwable;
+
+class JsonErrorController


Наверное это лучше вынести в слушатель ExceptionEvent'a. https://github.com/symfony/symfony/blob/6.0/src/Symfony/Component/HttpKernel/Event/ExceptionEvent.php

Искал пример с подобной реализацией (видимо плохо искал)... Спасибо.
Есть у такой реализации ещё профиты, кроме удобства и простоты?

Получается, любое внутреннее исключение отобразится пользователю, что не очень хорошо. Тот метод, который предложил @DaringFrogling c обработкой ошибок более гибкий, но практически равнозначен текущей реализации. Здесь же стоит добавить логирование ошибки и отображение пользователю общей информации о ней

lysiakin · 2021-12-21T22:42:43Z

src/Controller/LinksController.php

+     */
+    public function create(Request $request, ManagerRegistry $doctrine, UrlTitleService $titleService): Response
+    {
+        // code in style of fat controller, because of no time... Need to do validations, some service etc


Если нет времени, то проще сразу писать всё в сервисах же 😂

Согласен, вся бизнес логика и работа с БД должны быть выделены в отдельный слой

lysiakin · 2021-12-21T22:44:00Z

src/Controller/LinksController.php

+    {
+        // code in style of fat controller, because of no time... Need to do validations, some service etc
+        if (empty($request->get('long_url'))) {
+            throw new BadRequestHttpException('long_url is required');


Нецелесообразная проверка. Её надо передать в валидатор и он выдат validationexception.

lysiakin · 2021-12-21T22:46:22Z

src/Controller/LinksController.php

+        }
+
+        $link = new Link();
+        $link->setUrl($request->get('long_url')); //todo: need to validate url


Тонкий контроллер содержит минимум логики в себе. А именно просто подготовливает реквест данные для передачи в сервисный слой: валидирует, создаёт команду/квери/дто объекты, вызывает сервисный слой и отдаёт ответ.
https://martinfowler.com/eaaCatalog/serviceLayer.html
https://habr.com/ru/post/547510/
Пример реализации можно подсмотреть у меня
https://github.com/DaringFrogling/url-shorter/pull/1/files#diff-d39b69a8e9b69fba23cbb7dc9c08302e4a2418d6e7ffe2b2cc94895ac05f95f4 контроллер
https://github.com/DaringFrogling/url-shorter/pull/1/files#diff-abc82886f1d6f24ae786da382083a02d30cc178212dafa29a35886f10b3f7465 сервис

lysiakin · 2021-12-21T22:51:29Z

src/Controller/LinksController.php

+            $link->setTags($request->get('tags', []));
+        }
+
+        $entityManager = $doctrine->getManager();


За выборку данных отвечают репозитории. И только.

BOPOH · 2021-12-22T15:54:13Z

src/Controller/LinksController.php

+
+        $entityManager->flush();
+
+        $result = $doctrine->getRepository(Link::class)->findOneBy(['url' => $link->getUrl()]);


зачем это выполнять, если у тебя объект ссылки уже есть выше? Почему нельзя вернуть его? Он же даже с id уже будет

Перенял привычку с проекта, где нет менеджера...

BOPOH · 2021-12-22T15:59:40Z

src/Controller/LinksController.php

+    public function update(ManagerRegistry $doctrine, string $id): Response
+    {
+        $entityManager = $doctrine->getManager();
+        $link = $entityManager->getRepository(Link::class)->find($id);


для простого find() можно же сделать $entityManager->find(Link::class, $id);, а для кейса "по id из запроса грузим сущность" можно вообще в сигнатуре метода это указать и у тебя симфони автоматом все подгрузит:

public function update(?Link $link, ManagerRegistry $doctrine, string $id): Response { if (!$link) { throw $this->createNotFoundException( 'No product found for id '.$id ); } }

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

Это поведение где-нибудь настраивается?

не понял что должно "настраиваться", я говорил про https://symfony.com/doc/current/routing.html#parameter-conversion (и там ссылки есть где подробнее почитать можно)

Это поведение где-нибудь настраивается?

Такое поведение доступно благодаря SensioFrameworkExtraBundle для типов данных \DateTime и для Entity доктрины. Подробнее - в документации к конверторам

BOPOH · 2021-12-22T16:01:29Z

src/Controller/LinksController.php

+        if (!$link) {
+            throw $this->createNotFoundException(
+                'No product found for id '.$id
+            );
+        }


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

Насколько я видел, либо ошибка, либо no content отдают. Например, https://stripe.com/docs/api/invoices/delete

Это как раз вопрос про идемпотентность операции delete. Несмотря на разный код ответа, сам результат ответа должен будет одинаковый: записи ведь уже нет

BOPOH · 2021-12-22T16:04:23Z

src/Entity/Link.php

+     * @ORM\Id
+     * @ORM\GeneratedValue(strategy="CUSTOM")
+     * @ORM\CustomIdGenerator(class=ShuffleKeywordGenerator::class)
+     * @ORM\Column(type="string", length=200, unique=true)
+     */
+    private $keyword;


id по строке работает не очень, потом проблемы со скоростью можно наблюдать. Лучше uuid, либо автоинкремент

А есть, где почитать про проблемы со скоростью? Есть проект с такой логикой, там щас 12млн записей и просадок нет (может быть пока?)
Задумка была в том, чтобы сгенерированное слово и было уникальным ключом. Смысл в id, если оно не будет использоваться нигде.

я вот такую книжку читал: https://www.oreilly.com/library/view/high-performance-mysql/9780596101718/, но она старая уже, вроде вот это новое издание: https://mobile.twitter.com/tobias_petry/status/1472951375646625799?s=21

ну и раз "чтобы сгенерированное слово и было уникальным ключом", то сразу косяк: а если мы захотим поменять сокращатель? Либо старый себя изжил, либо хотим разные алгоритмы попробовать?
Если два разных логических значения задействуют одно и то же свойство, то однажды станет больно: надо будет разделить, а как понять, что в одном месте это id, а в другом короткая ссылка?

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

Технически для данной задачи всё ок, можно использовать shortUrl в качестве primary key и на скорость работы это никак не повлияет. Если же ожидается изменение структуры таблицы, бд или метода генерации ссылок, то проблемы, о которых пишет @BOPOH возникнут и их придётся решать.

BOPOH · 2021-12-22T16:15:03Z

src/Command/InitOptionsCommand.php

+        if (!$result) {
+            $nextId = new Option();
+            $nextId->setName('next_id');
+            $nextId->setValue('1');
+
+            $manager->persist($nextId);
+            $manager->flush();
+            $output->writeln('Done!');
+
+            return Command::SUCCESS;
+        } else {
+            $output->writeln('Option next_id already exists');
+
+            return Command::FAILURE;
+        }


ветка else в данном случае не нужна: если вошло в условие, то никогда из него уже не выйдет (потому что return)
было бы нагляднее переписать в следующем виде:

if ($result) { $output->writeln('Option next_id already exists'); return Command::FAILURE; } $nextId = new Option(); ... return Command::SUCCESS;

BOPOH · 2021-12-22T16:19:20Z

src/Service/SequentialKeywordGenerator.php

+use Doctrine\ORM\EntityManager;
+use Doctrine\ORM\Id\AbstractIdGenerator;
+
+class SequentialKeywordGenerator extends AbstractIdGenerator


очень похоже на то, что здесь ты пытался создать собственный автоинкремент на строках. А зачем переусложнять? Подозреваю, что из-за этого у тебя и падает всё.

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

Да, почти так и было, только это для строки с фиксированной длиной. И он не используется...

BOPOH · 2021-12-22T16:30:06Z

src/Service/UrlTitleService.php

+use Symfony\Component\DomCrawler\Crawler;
+use Symfony\Contracts\HttpClient\HttpClientInterface;
+
+class UrlTitleService


D из SOLID советует работать с абстракциями. В некоторых случаях, если вероятность изменения реализации очень мала, то можно поступить так, как реализовано:

создаем класс

везде его задействуем

и если появилась необходимость изменения реализации, то создаем интерфейс с именем текущего класса, а имя класса меняем на указание реализации (ну и интерфейс созданный указываем)
отсюда вытекает следующая мысль: имя класса должно быть абстрактным, без указания реализации: TitleService (ну и лучше указать для чего Title - например, LinkTitleService)

но в данном случае смена реализации более чем возможна, тем более имя класса само говорит об этом: url title, а значит имело смысл сразу выделить интерфейс и везде использовать его, а не конкретный класс

если же под url title имелось ввиду link title из задачи, то стоило так и назвать, а не плодить в одном контексте разные названия для одной и той же сущности

lis-dev

В целом следующие замечания:

Конфигурация: она дефолтная, но очень избыточная (в докерфайле лишние библиотеки, веб-сервер, mercure и т.п.).
Переусложнённая логика с Options и всё, что связано с этой Entity
Использование ManagerRegistry для получения и репозитория и EntityManager'a - лучше разделять их использование и внедрять как зависимость только то, что нужно и когда нужно
Кодстайл: php-cs-fixer поможет придерживаться общего стиля

lis-dev · 2021-12-24T07:26:42Z

.github/FUNDING.yml

@@ -0,0 +1 @@
+github: [ dunglas ]


Это, видимо, случайно попало в коммит

lis-dev · 2021-12-24T11:30:53Z

Dockerfile

+ENV SYMFONY_VERSION ${SYMFONY_VERSION}
+
+# Download the Symfony skeleton and leverage Docker cache layers
+RUN composer create-project "${SKELETON} ${SYMFONY_VERSION}" . --stability=$STABILITY --prefer-dist --no-dev --no-progress --no-interaction; \


Это действительно изменённый Dockerfile из рекомендаций symfony по работе с докером . Это нормальное решение, однако, как заметил @DaringFrogling, не атомарное, потому что код не отделён от инфраструктуры. Идеологически правильно было бы код хранить в отдельном контейнере и шарить его volume между контейнерами, но при этом, увеличивается сетевая нагрузка внутри докера, что будет добавлять неудобства при разработке (особенно касается macos)

lis-dev · 2021-12-24T11:44:19Z

Dockerfile

+	--with github.com/dunglas/mercure \
+	--with github.com/dunglas/mercure/caddy \
+	--with github.com/dunglas/vulcain \
+	--with github.com/dunglas/vulcain/caddy


mercure и vulcain , насколько я вижу, в приложении не используются

lis-dev · 2021-12-24T12:25:13Z

src/EventListener/RequestTransformerListener.php

+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\Event\RequestEvent;
+
+final class RequestTransformerListener


Т.к. здесь меняется тело реквеста при каждом запросе и используется sensio framework extra bundle, я бы рекомендовал использовать ParamConverter

lis-dev · 2021-12-24T12:32:22Z

config/services.yaml

+      - '../src/Kernel.php'
+      - '../src/Tests/'
+
+  App\EventListener\RequestTransformerListener:


Несмотря на некоторые преимущества EventListeners, я бы всё же рекомендовал использовать EventSubscribers. Они немного нагляднее и autowire поможет не прописывать каждый из них в конфигурации.

lis-dev · 2021-12-24T14:46:36Z

src/Repository/OptionRepository.php

+    {
+        $nextId = $this->findOneBy(['name' => 'next_id']);
+        if (!$nextId) {
+            throw new NextIdNotFound();


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

Частично соглашусь. NextIdNotFound - определяет конкретную ситуацию, а не класс возможных ситуаций. Было бы уместнее сделать, например, RequiredOptionNotFoundExcpeption('next_id not found'); .

lis-dev · 2021-12-24T14:52:09Z

src/Service/SequentialKeywordGenerator.php

+        LinkRepository $linkRepository,
+        string $charset,
+        int $keywordLength
+    ) {


В php начиная с версии 8.0 можно делать constructor property promotion:

public function __construct( private OptionRepository $optionRepository, private LinkRepository $linkRepository, private string $charset, private int $keywordLength ) {}

lis-dev · 2021-12-24T14:53:43Z

src/Service/SequentialKeywordGenerator.php

+
+    }
+
+    public function generate(EntityManager $em, $entity)


метод не использует ни $em, ни $entity

lis-dev · 2021-12-24T14:54:59Z

src/Service/SequentialKeywordGenerator.php

+        $charsetLen = strlen($this->charset);
+        while ($strLen < $this->keywordLength) {
+            while ($number >= $charsetLen) {
+                $mod = bcmod($number, (string)$charsetLen);


из-за declare(strict_types=1); здесь php должен ругаться, ведь bcmod принимает в параметрах string

lis-dev · 2021-12-24T14:58:11Z

src/Exception/NextIdNotFound.php

+
+use Symfony\Component\HttpKernel\Exception\HttpException;
+
+class NextIdNotFound extends HttpException


Наследование от HttpException неудачное, т.к. класс ошибки никак не связан с http.

Andrey Temchishen and others added 11 commits December 13, 2021 21:08

Init symfony skeleton

8867393

init symfony first page

78cd0ed

controllers and routes

2ec624a

fix routes

a921c3e

install postgre

46d4f89

Link and click entities

5e5c07b

add options

872d637

Link experiments and wip on jeyword generator

e10be5f

symfony code style

e36a488

shuffle and sequential generators

8939dad

json by default, experiments

cb29411

It's isnt work because memory leak in DebugClassLoader and AnnotationDriver

lysiakin reviewed Dec 21, 2021

View reviewed changes

BOPOH reviewed Dec 22, 2021

View reviewed changes

lis-dev reviewed Dec 24, 2021

View reviewed changes


		$entityManager->flush();

		$result = $doctrine->getRepository(Link::class)->findOneBy(['url' => $link->getUrl()]);


		use Symfony\Component\HttpKernel\Exception\HttpException;

		class NextIdNotFound extends HttpException

Задание 0 #1

Are you sure you want to change the base?

Задание 0 #1

Uh oh!

Conversation

AndrewTemchishen commented Dec 18, 2021

Uh oh!

YourCodeReview commented Dec 20, 2021

Uh oh!

lysiakin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lysiakin left a comment •

edited

Loading