«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2021/03/07 15:30:29  №1960025 1
Ответы: >>1960646
Аноним 2021/03/08 11:30:42  №1960646 2
>>1960025

> UNIQUE KEY `unique_index` (`email`,`token`)
Тут наверно надо 2 отдельных индекса - по email и по токену. А то у тебя, получается, можно внести несколько записей с одинаковым email, если у них разные токены.

> return json_decode(file_get_contents($file));
Советую добавлять флаг JSON_THROW_ON_ERROR, без него при неправильном JSON функция просто вернет null и не выдаст никакой ошибки (см. мануал по json_decode).

Также, мне кажется, JSON лучше преобразовывать в массив, а не в объект. Так как stdСlass в PHP - это непонятное недоразумение, это по сути не объект, а просто массив (так как у него не определен список полей и нет методов), только без функций для работы с ним. Зачем stdClass нужен, я не понимаю. Если ты хочешь мапить конфиг на объект, то сделай для этого класс и десериализуй JSON в объект этого класса. Если не хочешь, то наверно лучше будет использовать массив.

> $container->register('request'
Мне кажется, не очень логично регистрировать в контейнере реквест. Ведь в контейнере обычно регистрируют постоянно живующие сервисы, которые неизменны. Реквест же меняется на каждом запросе. Потому его нелогично класть в контейнер и вдвойне нелогично, если какие-то сервисы от него зависят. Представь, что у тебя неумирающий после каждого запроса сервер (или ты просто решил написать тесты). Захочешь ты обработать несколько разных реквестов и это не получится сделать.

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

Роутер логично положить в контейнер, но нелогично, что мы передаем реквест ему в конструктор. Правильнее сделать в нем метод route($request) и тогда один роутер сможет обработать любое количество реквестов. А вот список роутов обычно передают в конструктор роутера. Так роутер будет чуть более универсальным и в нем не будет захардкожен путь к конфигу (и ему вообще не важно будет, откуда берется конфиг).

Соответственно, в App мы бы создавали новый реквест и передавали в роутер. А тот передавал бы реквест и параметры роутинга в контроллер.

Класс Db не особо нужен. У меня такое ощущение, что в каких-то других учебниках зачем-то учат его делать и потому он появляется в коде в каждой работе. Класс Db (создание объекта PDO) полностью можно перенести в bootstrap.php. Ты ведь для других объектов не делаешь классы-фабрики, а для PDO почему-то сделал.

> @var DIContainer
> private DIContainer $container;

> @param DIContainer $container
> public function __construct(DIContainer $container)

> @param int $statusCode
> @return Response
> private function createResponse(int $statusCode = 200): Response

Не стоит писать phpDoc, если он не несет никакой новой информации, а просто дублирует указанный в коде тип.

> error_log($exception->__toString());
Ошибки типа 404 можно не логгировать (они все равно логгируются на веб-сервере вроде nginx, если нужно).

Насчет исключений, я не думаю, что все исключения должны наследовать ApplicationException. Как я понял, этот класс обозначает исключение, к которому привязан определенный HTTP код и сообщение на странице ошибки. Но если взять тот же ContainerException (или DbException), какое отношение контейнер имеет к HTTP-ошибкам? Никакого. Контейнер не должен решать, какую страницу ошибки выводить и что на ней писать, он должен просто сигнализировать об ошибке и все. Потому он не должен наследоваться от ApplicationException. От него должны наследоваться только исключения вроде ошибок доступа (код 403), ошибки 404 и тд.

Получается нарушение принципа единой ответственности. Контейнер или БД, выбрасывая исключение, определяют, какую страницу ошибки показать, с каким кодом и каким сообщением. Это не их ответственность.

> $split = $this->getSplitRealPath();
> $request->setRequestBody($split);

Этот момент в роутере выглядит коряво. Почему-то значения GET/POST проставляются в Request только после роутинга. Логичней проставлять их при создании Request, а позже, при необходимости, добавлять значения параметров через $request->setAttributes. Хотя, мне не очень нравится, что в реквест вообще что-то дописывается. По идее же реквест представляет пришедший от пользователя запрос, а нем нет никаких атрибутов. В нем есть URL, GET, POST и все. Я знаю, что некоторые фреймворки дописывают атрибуты в реквест, но это, на мой взгляд, выглядит коряво. Логичнее было бы параметры после роутинга не записывать в реквест, а просто передавать в контроллер.

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

actionIndex и actionSearch можно было бы объединить вместе, это очень похожие вещи - выборка из таблицы.

> private function getHtml(string $items): string

Это конечно корявенько, что HTML формируется не через шаблон. Можно было сделать, чтобы Pagination просто возвращал бы данные для отображения пагинатора, а HTML формировать в шаблоне из этих данных.

Неаккуратно сделано, что CookieHelper работает в обход Request/Response и ставит куки напрямую.

> public function getCsrfToken(): string
> return ($this->cookie->getCookie('csrf')) ?: $this->csrfToken;
Логика работы этой функции непонятна. Она возвращает непонятно что.

> if ($_SERVER['REQUEST_METHOD'] == 'POST') {
Это логичнее делать через $request->isPost().

> private int $outputRows;
Мне кажется, это стоило просто прописать в контроллере. StudentTableGateway не отвечает за отображение данных и не знает, по сколько строк надо выводить.

В функции getAll() наверно не надо было заморачиваться с джойном. Обычно делают так, что пагинация работает после сортировки, то есть сначала записи сортируются и затем из них выбирается запрошенная страница. Просто ORDER BY ... LIMIT ....

Не очень хорошо, что разные функции возвращают объекты Student с разным набором полей. Это может вызвать потом путаницу в коде, так как непонятно, как различать эти объекты. Что, если ты передашь "неполноценный" объект в функцию, которой нужно какое-то поле, которого в нем нет?

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

> ?key=(id|name|surname|sgroup|score)&sort=(asc|desc)(&page=[1-9][0-9])?

Обычно при роутинге GET-параметры игнорируют и смотрят только на path.

> (isset($student->gender) && $student->gender == 'male'
Тут стоило использовать константу, наверно.

В общем, сделано хорошо. Видно, что ты поковырялся в сторонних фреймворках.

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

Ответы: >>1960740 >>1960782 >>1962389
Аноним 2021/03/08 12:22:21  №1960740 3
>>1960646

> Из-за этого у тебя Request и Router получаются спутаны друг с другом, а лучше бы если они были независимы.
Меня этот момент тоже напрягал, но что-то было лень переписывать, ибо пару раз приходилось переписывать часть логики, т.к. заранее не продумал + не так много опыта, а хотелось побыстрее закончить

> Это конечно корявенько, что HTML формируется не через шаблон.
Ты имеешь в виду, чтобы сделать отдельный шаблон для пагинатора, а сам пагинатор просто будет возвращать <ul> с необходимыми элементами ?

> В функции getAll() наверно не надо было заморачиваться с джойном.
Как раз и сделал через джойны для пагинации. Чтобы происходила сортировка только для тех данных, которые отображаются на странице. Если делать без джойнов, то получится, что сначала выполняется сортировка, а только потом LIMIT X, Y. Данные на N странице одни, а после сортировки другие. Как по-другому сделать (если вообще можно по-другому сделать на стороне бека) - не знаю

Спасибо большое, Анон, что указал на косяки и что можно исправить. Как думаешь, можно уже пытаться по собесам походить ?
Ответы: >>1961264
Аноним 2021/03/08 12:49:07  №1960782 4
>>1960646

> Этот момент в роутере выглядит коряво. Почему-то значения GET/POST проставляются в Request только после роутинга.
Вспомнил, почему так вообще получилось. Получилось так из-за того, Request разбирает не только GET/POST, но еще и может разбирать параметры из маршрутов, н.р маршрут:
user/(id/[0-9]+) => user/actionView/$1
Соответственно, чтобы разобрать параметры, которые заданы через маршрут, ему нужно дергать метод роутера (getSplitRealPath), который как раз получает путь user/actionView/$1 на основе запрошенного маршрута. По итогу придется либо отказываться от этого, либо будут повторяющиеся методы в разных классах, ну или зависимость
Ответы: >>1961264
Аноним 2021/03/08 16:14:26  №1961264 5
>>1960740

> Ты имеешь в виду, чтобы сделать отдельный шаблон для пагинатора, а сам пагинатор просто будет возвращать <ul> с необходимыми элементами ?

Можно сделать так: делаем отдельный HTML-шаблон и в него передаем объект Paginator. Шаблон выводит HTML, а за подробностями (какие цифры и ссылки выводить) обращается к Paginator.

Генерация HTML в коде плохая идея, так как такую верстку трудно поддерживать, разбираться в ней, вносить правки.

> Если делать без джойнов, то получится, что сначала выполняется сортировка, а только потом LIMIT X, Y. Данные на N странице одни, а после сортировки другие. Как по-другому сделать (если вообще можно по-другому сделать на стороне бека) - не знаю

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

И еще, обычно при смене сортировки пагинация сбрасывается на первую страницу.

> Как думаешь, можно уже пытаться по собесам походить ?

Я не HR. Посмотри требования к вакансиям. Согласись сделать тестовое задание, возможно оно по сложности будет как эта задача или проще.

>>1960782

> Получилось так из-за того, Request разбирает не только GET/POST, но еще и может разбирать параметры из маршрутов, н.р маршрут:
> user/(id/[0-9]+) => user/actionView/$1

Это $1 в Симфони называют "атрибутами" и роутер после разбора URL помещает их в $request->attributes. Ты бы мог тоже сделать в реквесте массив для атрибутов и методы get/set для него. Но мне кажется, это коряво. Получается, объект Request знает что-то о роутинге, хотя он должен лишь представлять полученные от пользователя данные. Было бы корректнее передавать атрибуты вроде $1 в метод контроллера, например так:

public function actionView(Request $r, array $attributes)

Или даже так, с использованием рефлексии по имени параметра определять, что туда передать:

public function actionView(Request $r, int $id)

В Симфони атрибуты реквеста используются и для других вещей - например, код может до вызова контроллера определить город пользователя и сохранить в атрибутах. А контроллер его оттуда возьмет. Но опять же, мне кажется, это коряво, а реквест из-за атрибутов превращается в свалку нетипизированных данных.
Ответы: >>1961327
Аноним 2021/03/08 16:36:58  №1961327 6
>>1961264

> Так и должно быть, что данные другие.
Понял. Но как ты смотришь на то, чтобы подзапрос в виде джойна оставить ради оптимизации лимита ? Читал статью (на хабре вроде), что если данных очень много и у нас будет LIMIT 100000, 10 например, то это сильно бьет по производительности. В плане того, что mysql сначала переберет 100000 записей, а только потом выдаст следующие 10

> Это $1 в Симфони называют "атрибутами" и роутер после разбора URL помещает их в $request->attributes.
После прошлого ответа еще посидел подумал и в голову похожая идея пришла. В плане того, что раз это параметры маршрутов, то и разбирать их должен непосредственно сам роутер, а не реквест. Значит в правильную сторону мыслил
Ответы: >>1962175
Аноним 2021/03/09 16:41:28  №1962175 7
>>1961327

> Но как ты смотришь на то, чтобы подзапрос в виде джойна оставить ради оптимизации лимита ?

Такая проблема производительности лимита есть. Она решается отказом от номеров страниц и использованием индекса, как описано тут https://use-the-index-luke.com/sql/partial-results/fetch-next-page

То есть, вместо LIMIT 10000, 10 мы используем, например, WHERE id > :lastId LIMIT 10.

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

У тебя сортировка сейчас, как мне кажется, сделана некорректно. При смене сортировки результаты на странице (например, первой) должны меняться, а у тебя они не меняются.
Ответы: >>1962389
Аноним 2021/03/09 20:00:30  №1962389 8
>>1960646

Анон, я пофиксил. Можешь еще раз посмотреть ?
https://github.com/qqzooi/student-list

>>1962175

>У тебя сортировка сейчас, как мне кажется, сделана некорректно.
Да, я уже исправил. Просто изначально думал, что должны сортироваться только те данные, которые в данный момент юзер видит на конкретной странице.
Ответы: >>1964928
Аноним 2021/03/12 13:17:47  №1964928 9
>>1962389

В роутере сделаны неудачно, на мой взгляд, методы getController() и getAction(). Чтобы они вернули значение, надо сначала вызвать route(), но как об этом догадаться? Получается, один метод при вызове влияет на работу других методов, это называется побочные эффекты и это нехорошо.

Правильнее было бы, если бы был метод, который получает, например, Request или просто $urlPath и возвращает controller, action и attributes.

Та же проблема с пагинацией. Было бы лучше, если бы все методы работали корректно без необходимости вызывать метод run().

> private int $limitStudents = 10;
Это можно было сделать private константой.

Плохо, что тут идет вперемешку PHP и HTML: https://github.com/qqzooi/student-list/blob/6466cbedf7026440a2885c871cc93701a0581cce/App/Components/Navbar.php#L7

Лучше было бы, если бы Navbar только хранил информацию о пунктах меню, а преобразование в HTML происходило бы в шаблоне. То есть, HTML-шаблон бы обращался к классу Navbar, получал список пунктов и рендерил их. В твоем решении править верстку неудобно, так как она раскидана кусками по классу.

То же касается NotificationUtil и других мест, где есть HTML в классах. HTML-код лучше формировать в шаблоне.

SVG-код, наверно лучше поместить в файл и подключать через file_get_contents(). Как в твоем коде редактировать иконки? Руками переносить в файл, потом обратно в код?

Я думаю, что JOIN в getAll() не нужен, так как он ни на что не влияет.

> user-scalable=no,
Это в общем плохое решение, так как лишает пользователя удобства. Может, у человека шрифт слишком мелкий или наоборот, мало информации на экран влезает. Я на десктопе часто на сайтах меняю масштаб.