«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2018/10/27 16:45:45  №1285501 1
Анон, который с https://github.com/Awesome-Kirill/fukingStudent - надо сделать папку для публичных файлов. Сейчас ты корень проекта вываливаешь в веб, это небезопасно. В комментариях к задаче это описано. Также, файлы для докер имеет смысл поместить в проект (можно внутрь отдельной папки), это неудобно, поддерживать 2 связанных репозитория.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/composer.json#L2
Название проекта можно убрать или вписать правильное, это поле обязательно только для библиотек.

Ты используешь https://github.com/Awesome-Kirill/autowiringDI , но в нем нет даже общего описания. Я первый раз эту библиотеку вижу. То есть человек, получается, должен читать код, чтобы понять, что это такое и как работает. Думаю, тут нужно дополнить ридми, кратко описав, что это, зачем, как работает с примером кода, что может и что не может. Кратко.

> Created by PhpStorm.
Советую отключить этот бессмысленный комментарий в настройках IDE.

> $this->container["{$id}"];
Красивее сделать явное преобразование к строке, как мне кажется, через strval().

> https://github.com/Awesome-Kirill/autowiringDI/blob/master/src/AutowiringDI.php#L66
> public function make(string $cls){
Нужен комментарий. Название ничего не говорит.

Тут выравнивание кривое из-за смеси табов и пробелов: https://github.com/Awesome-Kirill/autowiringDI/blob/master/src/AutowiringDI.php - надо исправить. Тяжело читать.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/student.sql#L40
> `ege` int(11) NOT NULL,
Нужно добавить UNSIGNED

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/mylog.log
Это надо добавить в gitignore, и логи лучше вынести в отдельную папку.

Твой роутер правильнее назвать Front Controller.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/tmp/404.html#L18
надо экранировать вывод, почитай мою пасту про XSS

https://github.com/Awesome-Kirill/fukingStudent/tree/master/src/tmp
Папке надо дать название получше

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/View.php
надо убрать лишние пустые строки в коде

> https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/Controller/FormController.php#L36
> $postRequest = $postRequest + ['pass' => hash('sha256', random_int(0, 1200))];
Ох ты наивняша. У тебя ведь, получается, тут всего 1200 вариантов пароля. Хеш может выглядеть внушительно, но в его основе лежит одно из всего лишь 1200 чисел. И подобрать такой пароль элементарно.

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

Ты, возможно, где-то слышал, что надо хешировать пароли. Это немного не то, что ты сделал, у меня есть урок: https://github.com/codedokode/pasta/blob/master/security/password-hashing.md

Пароли хешируют и солят, чтобы персонал не мог их подглядеть, а также, чтобы при краже БД нельзя было взять эти пароли и попробовать их ввести на том же самом или других сайтах - вдруг пользователь использует тот же пароль. Тут нужен не пароль (который вводит пользователь), а случайный токен, который сохраняется в куки. Хешировать его не требуется (но можно при желании), так как он не подойдет ни к какому другому сайту.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/Model/DataBase.php#L23
> } catch (\PDOException $e) {
> echo $e->getMessage();
Непраивльно. Ты выводишь ошибку пользователю (который ничего в ней не поймет), а в лог не пишешь (и разработчик о ней не узнает).

> https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/Model/DataBase.php#L123
> $config = parse_ini_file('./src/config.ini');
Почему у тебя класс базы данных занимается парсингом конфигов? Ты нарушил принцип разделения ответственности, когда каждый класс занимается своим делом. Ну и представь, если тебе захочется, например, усложнить код чтения конфига. Тебе придется копировать это в несколько мест. Неудобно.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/Model/InitPDO.php
Непонятно назначение этого класса. Что мешает вместо него создавать PDO напрямую?

Также, ты там сделал логгирование. Ты для каждого catch в коде планируешь копировать код логгирования?

Еще, ты для модели студента используешь массив вместо объекта. Можешь назвать плюсы и минусы представления студента в виде массива или объекта? Почему ты так решил сделать?

Класс валидации сделан не очень удачно - надо вызвать один метод для проверки, потом другой для получения результата. При этом ошибки накапливаются, если например, вызвать метод проверки второй раз, то старые ошибки не очищаются. Вопрос, почему нельзя вернуть результат проверки сразу? Зачем так сделано?

По тестам: не стоит делать проверку кучи условий в одном тесте. Лучше на каждое условие сделать свой тест:

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

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

> $I->see('Этот email уже использован');
Это делает тест хрупким. Достаточно чуть поменять формулировку и все, тест провален. Лучше бы тестировать по более надежному признаку, может там есть какой-то класс или атрибут, который менее склонен к изменению.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/tests/acceptance/FirstCest.php#L27
> $I->see('Logout');

Это тоже не очень надежно, вдруг это слово где-то есть в фразе на странице (you can logout later). Стоит хотя бы проверить до теста, что такого слова нет - тогда мы защитимся от этой ошибки.

> $I->amOnPage('/add/');
> $I->dontSeeCookie('isLogin');
Это мне не очень нравится, так как пользователь таких вещей не видит, а мы ведь имитируем его поведение. Также, ты привязываешься к подробностям реализации. То есть ты не проверяешь, что пользователь разлогинен, а проверяешь, что нет куки с определенным именем. И если ее имя поменяется, твой тест станет ошибаться.

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

https://github.com/Awesome-Kirill/fukingStudent/blob/master/tests/acceptance.suite.yml
Надо написать в ридми про этот файл, что в нем надо задать параметры соединения. И вообще про тесты, как их запустить.

Это не полный список замечаний, это только что я с первого взгляда увидел. Задавай вопросы, если что
-то непонятно.
Ответы: >>1285903
Аноним 2018/10/28 12:40:51  №1285903 2
>>1283051

В дополнение к замечаниям тут: >>1285501

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

Имена шаблонов лучше сделать соответствующими именам контроллеров и экшенов, а не случайными.

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/Controller/ListController.php#L36
> if (isset($_COOKIE['isLogin']) and $this->authentication->isValidCookie($this->model, $_COOKIE['isLogin'])) {

Это нарушение инкапсуляции. По идее, знать, как называется кука, должен только класс авторизации. Ты же вместо этого копипастишь название куки по всему коду.

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

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/tmp/allVue.html#L6
> <meta name="viewport" content="initial-scale=1, maximum-scale=1, user-scalable=no">

Зачем ты запрещаешь масштабировать страницу? Этот код придумали люди, которым лень было делать нормальную верстку и проще показалось запретить масштабирование. Я негативно отношусь к таким людям.

> <hr class="container-fluid">
Что за ерунда? Почему hr?

https://github.com/Awesome-Kirill/fukingStudent/blob/master/src/tmp/vue-bootstrap-table.js
12 000 строк - не многовато ли для отображения простой таблицы?

Фреймворки вроде bootstrap лучше класть в отдельную папку, а не сваливать все в кучу. Сторонний код желательно явно отделять от своего.

Весь бутстрап тебе вряд ли нужен. Можно было сделать сборку только из нужных модулей.

При ошибке надо показывать форму с заполненными полями, а не редиректить.