«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2019/03/03 09:52:48  №1358261 1
Ответы: >>1358314 >>1369057
Аноним 2019/03/03 11:17:19  №1358314 2
>>1358261
Для начала сделай нормальную автозагрузку
https://github.com/asdasdasdasddasasdasdas/StudentList Аноним 2019/03/23 20:15:07  №1369057 3
>>1358261

> https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/students.sql#L24
> `hash` varchar(100) NOT NULL,

Здесь стоит добавить комментарий (COMMENT ...) с пояснением, что в этой колонке.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/bootstrap.php

Ты сделал DI так, что для контроллера он возвращает функцию-фабрику, которую надо вызвать, чтобы получить сам контроллер. Но лучше было бы решить это на уровне самого DI контейнера, например, сделать отдельный метод bindFactory, чтобы указать, что этот объект создается функцией. А получатель просто вызывает $di->get() и не задумывается о том, как именно создан объект.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/helpers/DIContainer.php#L37
Нет смысла писать throw и catch рядом. Ты можешь заменить их на обычный if, и код будет проще читать.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/exceptions/DIContainerException.php
Тут ошибка. Исключение - это объект, хранящий информацию об ошибке. Но в нем нет обработчика, который решает, что делать в случае ее возникновения. Если ты хочешь при ошибке показывать страницу, то проще всего ловить ошибку где-то в роутере, и выводить страницу ошибки. Не забудь также логгировать исключение, например, с помощью error_log(). В твоем варианте разработчик даже не узнает про ошибку.

Урок про исключения: https://github.com/codedokode/pasta/blob/master/php/exceptions.md

В роутере мне не нравится, что у тебя у одной страницы может быть много URL, например: /registration, /registration/xyz. Это плохо для поисковой оптимизации.

Сообщения о коммитах плохие: https://github.com/asdasdasdasddasasdasdas/StudentList/commits/ads

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/MainController.php#L30
> $countStudents = $_GET['search'] !== '' ?

Здесь при отсутствии ключа search в массиве генерируется notice. Странно, что ты его не заметил - может, у тебя отключен их показ?

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/model/StudentTableGateway.php
Здесь стоило использовать DI для передачи соединения с БД снаружи. Класс DBConnector тогда становится не нужен.

В коде очень мало комментариев. Понятно, что многие места очевидны, но перед некоторыми классами вроде Paginator и некоторыми публичными методами стоит их добавлять. Перед методами контроллеров удобно писать, за какую страницу они отвечают. Заодно почитай про phpDoc.

Стоит добавлять тайп-хинты (string, int) в аргументы функций. Это защищает от ошибок и документирует код.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/model/StudentTableGateway.php#L70
Здесь в getStudents(int $offset, int $limit) в SQL запросе ты не указал способ сортировки. Это значит, что БД может возвращать любые записи, например, одни и те же записи для любой страницы. То есть конструкция OFFSET без сортировки не имеет особого смысла.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/model/StudentTableGateway.php#L143
Проверка сделана некорректно тут:

> } elseif ($result['id'] == $id && $result['email'] == $email) {

Может быть такое, что найдется 2 записи, но первой вернется запись с нужным id, а вторую ты не проверяешь.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/controller/ProfileController.php#L83
тут можно было сделать вспомогательную функцию, чтобы не копипастить длинные выражения.

> $student = $this->auth->getAuthUser($this->auth->getHash());
Странно, что мы должны передавать хеш, хотя $this->auth его и так знает.

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/helpers/Authorization.php#L22
> public function IsLoggedIn()
> {
> return isset($_COOKIE['hash']) ? true : false;
Этой проверки недостаточно, чтобы считать пользователя залогиненным. Что, если он сам себе поставит куку hash=1 ?

https://github.com/asdasdasdasddasasdasdas/StudentList/blob/ads/app/view/partials/form.php
Тут стоит добавить атрибуты для HTML5 валидации до отправки формы.

> <?= $student->gender == 'f' ? 'checked=checked' : null; ?>
echo выводит только числа и строки, неправильно передавать на вывод null

Радует, что ты прочитал про SQL-инъекции и XSS. А есть ли у тебя защита от XSRF?

> if ($this->paginator->getPreviousPage() !== null
Красивее писать if ($this->paginator->hasPreviousPage()), а еще лучше $paginator->hasPreviousPage()

При ошибке 404 надо отдавать соотв. HTTP-код. То же самое касается страницы ошибки в приложении.

Сессии наверно не нужны в index.php?

Вообще, пока впечатление по коду хорошее.