-
Notifications
You must be signed in to change notification settings - Fork 10
Lesson 2.8 #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Lesson 2.8 #3
Conversation
src/Controller/ProjectController.php
Outdated
| public function list(Request $request): Response | ||
| { | ||
| $user = $this -> getUser(); | ||
| $project_repository = $this -> getDoctrine() -> getManager() -> getRepository(Project::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Разделение пробелами следует убрать, такой способ форматирования не используется ни в одном известном Code Style Guide.
- underscore (использование подчеркивания при именовании переменных) в целом в рамках PHP имеет место быть. Но в рамках Symfony принят формат с camelCase
Подробнее про Code Style тут https://symfony.com/doc/current/contributing/code/standards.html
src/Controller/ProjectController.php
Outdated
| $user = $this -> getUser(); | ||
| $project_repository = $this -> getDoctrine() -> getManager() -> getRepository(Project::class); | ||
|
|
||
| if (in_array("ROLE_ADMIN", $user -> getRoles())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Проверку на роль лучше делать через метод $this->isGranted('ROLE_ADMIN')
-
Выделить в отдельный метод репозитория для получения списка доступных объектов.
src/Controller/ProjectController.php
Outdated
|
|
||
| return $this -> render("projects/create.html.twig", [ | ||
| "form" => $form -> createView(), | ||
| "action" => "creating" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут подойдет Create.
ing - это продолжительное действие (например, loading, creating, waiting - загрузка, создание, ожидание) и тп
лучше использовать для действия инфинитив, Create, Show, Delete, Get и тп
src/Controller/ProjectController.php
Outdated
| * @Route("/projects/{slug}", name="project_info") | ||
| * @return Response | ||
| */ | ||
| public function info($slug, Request $request): Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Когда есть контроллер для вывода конкретной сущности лучше использовать устоявщиеся названия методов:
Например:
get
show
present
Тогда логино выглядит $projectController::show($slug) или get($slug)
src/Controller/ProjectController.php
Outdated
| { | ||
| $project = $this -> getDoctrine() -> getManager() -> find(Project::class, $slug); | ||
|
|
||
| $tasks = $this -> getDoctrine() -> getRepository(Task::class) -> findBy(["project" => $project]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Лучше использовать более короткий и выразительный вариант:
$tasks = $project->getTasks()
src/Controller/TaskController.php
Outdated
| $project_repository = $this -> getDoctrine() -> getRepository(Project::class); | ||
| $task_repository = $this->getDoctrine()->getRepository(Task::class); | ||
|
|
||
| $projects_list = $user_is_admin ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вынести в метод репозитория (выше про это было)
src/Controller/TaskController.php
Outdated
| $filter = $taskFilterForm->getData(); | ||
| if ($filter['isCompleted'] === null) { | ||
| unset($filter['isCompleted']); | ||
| $data = $taskFilterForm->getData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Все что ниже нужно скрыть в самом методе фильтрации репозитория.
src/Controller/TaskController.php
Outdated
| $project_repository = $this -> getDoctrine() -> getRepository(Project::class); | ||
|
|
||
| $form = $this->createForm(TaskType::class, data: $task, options: [ | ||
| "projects_list" => in_array("ROLE_ADMIN", $user -> getRoles()) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В отдельный метод
src/Entity/User.php
Outdated
| * @param string $role | ||
| * @return bool | ||
| */ | ||
| public function checkRole(string $role): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasRole лучше
src/Repository/TaskRepository.php
Outdated
| /** | ||
| * @throws Exception | ||
| */ | ||
| public function getTasksByProjectOwnerId(int $id) : array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Такая реализация имеет место быть, но несколько нивелирует преимущества использования ORM:
- во-первых возвращается ассоциативный массив, то есть мы не можем использовать функции Collection/и методы сущностей Task/Project.
- во-вторых в случае добавления полей в БД/удаления/Переименования придется изменить все SQL запросы
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Изменить реализацию через QueryBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…грамме, чтобы было удобнее проверять :)
selezenev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно добавить в контроллерах и шаблонах проверку на доступность.
Реализовать через Voter
| * @Route("/projects/{slug}", name="project_show") | ||
| * @return Response | ||
| */ | ||
| public function show($slug, Request $request): Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нет проверки что проект доступен пользователю.
Нужно добавить, реализовать через Voter
| * @param Request $request | ||
| * @return Response | ||
| */ | ||
| public function edit($slug, Request $request): Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Также нет проверки на доступность
| */ | ||
| public function edit($id, Request $request): Response | ||
| { | ||
| $task = $this->getDoctrine()->getManager()->find(Task::class, $id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нет проверок на доступность и права
| $projectsList = $projectRepository -> findByUserRole($user -> getId()); | ||
| $projectsIdList = array_map(function ($item) { return $item->getId(); }, $projectsList); | ||
|
|
||
| $taskFilterForm = $this->createForm(TaskFilterType::class, options: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Давай заменим ChoiceType на EntityType чтобы скрыть логику получения списков из контроллера в форму
|
|
||
| $form = $this->createForm(TaskType::class, $task, [ | ||
| "projectsList" => $projectRepository -> findByUserRole($user -> getId()), | ||
| // 'userId' => 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Закомменитированный код ненужно оставлять
Выполнил все задания из ридми, всё в рабочем состоянии