Комментарии 22
В целом интересно, хотя на мой взгляд немного сумбурно. Причем как в статье, так и в коде. Я понимаю, что пример небольшой, но файл routes.php получился какой-то и швец, и жнец и на дуде игрец. Плюс стандартное замечание про vendor в гите.
Не совсем понятно про обработку ошибок. В частности, пустой try-catch в ProccessRawBody (во-первых, непонятен смысл пустого try-catch, а во-вторых, вроде бы, json_decode сам по себе исключения не кидает, а только с определённым флагом). Не лучше ли было бы переписать как-то так:
if ($rawBody) {
$body = json_decode($rawBody, true, 512, JSON_THROW_ON_ERROR);
foreach ($body as $key => $value) {
$request->$key = $value;
}
}И по функции-обработчику, я бы поменял Exception::class на Throwable::class и добавил бы логирование в PROD режиме, а то на бою долго придется причину ошибки искать.
Да, получилось сумбурно). Не хотел очень в подробности закапываться, чтобы сам подход остался виден. В конечном итоге это просто использование "pecee/simple-router" или аналогичной библиотеки + немного ООП стиля.
я бы поменял Exception::class на Throwable::class
Да, я тоже так подумал, но эта библиотека, как я понял, выбрасывает именно Exception. Сам этот код из их примера.
Но сам-то РНР выбрасывает и те и другие. И если было брошено Throwable, то статус тоже должен быть 500, а не 200.
Да, но сюда Router::error() оно не попадёт.
Для общего случая подойдёт:
try {
Router::start();
} catch (Throwable $e) {
}Но я ориентировался на целевую аудиторию фронтендеров и тех кто не гнушается простыми php файлами для создания сайта, поэтому такие вещи думаю выходят за рамки.
Прекрасно всё попадёт. И даже если представить, что не не попадёт, то чем Throwable помешает?
И опять этот пустой try-catch, который в сто раз хуже. Это, блин, как страус, который голову в песок воткнул, и думает что если он хищника не видит, то и хищник его не видит. Если я ошибок не вижу, то их типа и нет?
В данном случае я не вижу, что здесь "выходит за рамки". Убрать try-catch? не убирать, но добавить хотя бы error_log? Сделать код в handle осмысленным, чтобы он не пытался ловить исключение, которое никто и не выбрасывал?
Прекрасно всё попадёт.
Ну, я тут не спорю с вашим посылом, потому что сам думал аналогично, и добавлял Throwable туда, но ничего не приходило.
Сейчас я глянул исходники и там прописано Exception.
/**
* @param Request $request
* @param Exception $error
*/
public function handleError(Request $request, Exception $error): void
{
/* Fire exceptions */
call_user_func($this->callback,
$request,
$error
);
}И опять этот пустой try-catch, который в сто раз хуже. Это, блин, как страус, который голову в песок воткнул, и думает что если он хищника не видит, то и хищник его не видит. Если я ошибок не вижу, то их типа и нет?
Пустой try-catch для того чтобы на публику не выбрасывалось описание ошибки, ибо чего там только может не быть. Вплоть до паролей. Тут не фреймворк и в прод окружении само собой исключения не пропадают (это магия? :-))
Поэтому считаю этот try-catch must have хоть и пустой.
В данном случае я не вижу, что здесь "выходит за рамки". Убрать try-catch? не убирать, но добавить хотя бы error_log? Сделать код в handle осмысленным, чтобы он не пытался ловить исключение, которое никто и не выбрасывал?
вот это всё и выходит :)))
Я не силён в этой теме.
Почему я считаю что пустой try-catch нужен я написал в другом ответе.
Пс. Тут недавно узнал что можно задать свой обработчик исключений через set_error_handler. Но опять же...
Ну вот "я сам не силён" это гораздо честнее, чем "я пишу для тех, кто не силён, поэтому попроще" )))
Пустой try-catch — это не "must have", а дурость. За которую очень больно бьют. Это полное, стопроцентное отсутствие опыта. Потому что любой мало-мальчки опытный программист быстро учится ценить ошибки на вес золота.
Поэтому для тех, у кого своего опыта ещё нету: если произошла ошибка, то программист должен её увидеть. Должен. Увидеть. Несмотря ни на что. Чтобы не сидеть, тупить в пустой экран, когда что-то не работает. потому что именно для этого ошибки и придуманы — чтобы сообщить программисту, что с его программой не так. А не для того, чтобы такие вот, которые "не сильны", не глядя их на помойку выбрасывали.
А с публикой всё ещё смешнее. Надо просто не путать выброс ошибки и её отображение. Выброс должен быть всегда. Отображение — по потребности.
Достаточно всего лишь поставить в настройках РНР display_errors=0, и никаких "паролей" никакая "публика" не увидит. При этом у программиста возможность увидеть ошибку сохранится. Поэтому никаких пустых try-catch и прочих радостей, типа @, error_reporting(0) в коде быть не должно.
Спасибо за "display_errors".
try-catch — это не "must have", а дурость.
Хорошо. Я плохо ориентируюсь в ... (не знаю к чему относится "display_errors", настройка php?).
Да, это одна из настроек php.ini
Она, разумеется, на любом боевом сервере должна быть выключена.
Только поправочка — не "try-catch", а именно пустой try-catch — это дурость.
Сам по себе try-catch бывает полезен, но только не для управления отображением ошибок.
По поводу set_error_handler. Это очень хорошее решение, позволяет избавиться от всего этого зоопарка из try-catch. Но ей одной не обойдёшься, нужно ещё set_exception_handler(), и, по-хорошему, register_shutdown_function(), чтобы ловить фатальные ошибки. Вот есть статья с примером, https://phpdelusions.net/articles/error_reporting
Здорово всё проясняет. Я всё хочу её на Хабр перевести, но никак не соберусь.
Кстати, изначально я сделал не пустой try-catch, и только потом увидел что там есть их родной обработчик и решил использовать его.
Конечно, этой теме не учат в школе, поэтому получается так как получается, пока не найдётся кто-то кто по рукам даст :).
вот что я начинал делать тут.
try {
Router::start();
} catch (\app\exceptions\NotAuthorizedHttpException $e) {
$response = Router::response();
$response->httpCode(401);
} catch (InvalidTokenStructure $e) {
$response = Router::response();
return $response->json([
'message' => $e->getMessage()
]);
} catch (Error $e) {
$response = Router::response();
$response->httpCode(500);
if (!PROD) {
return $response->json([
'status' => 'error',
'code' => 500,
'message' => $e->getMessage()
]);
}
}Ну кстати вот сильно лучше чем у него. Только всё это лучше писать в set_exception_handler — просто чтобы вынести это всё в отдельный файл/класс, а не городить прямо в индекс.
Только ловить, опять же, не Error а Throwable, как самое высокоуровневое
Плюс надо добавить логирование, поскольку если мы сами обрабатываем ошибку, то РНР её уже не залогирует.
И в ПРОД режиме я бы отправлял не пустой массив, а всё-таки что-то осмысленное. 'status' => 'Server error' и 'code' => 500 по-любому можно отдать.
файл routes.php получился какой-то и швец, и жнец и на дуде игрец
Именно этим мне не понравилось использование этой библиотеки. Тут, как я вижу, остаётся только выносить роуты в массив и динамически создавать их, но я пока отказался от развития этого направления, потому что не смог пока интегрировать сюда контейнер зависимостей.
Интересно, имеет место быть для развития. Практически лучше Symfony MicroKernel (да, сложнее, но минимализм + возможность расширения + документация) или Lumen (говорят, что умер, но на проекте используем для API - вполне).
Вопрос по коду: если вы захотите использовать Фасады - то сделаете еще папку facades? А сервисы поместите в корень в services? А если кодовая база начнет расти? Может вся логика приложения могла бы уйти в папку app как у Laravel?
https://github.com/i-luka/api-php/blob/revision1/config/routes.php
строка 55: как-то неочевидно константа PROD в глобальном пространстве. Удобнее вынести в env конфиг
Можно разделить Router на App, Kernel и Route. Тогда в файле index.php будет создаваться инстанс App, который будет подгружать ядро, которое будет реализовывать логику роутера.
Почему не компоненты symfony используете?
если вы захотите использовать Фасады - то сделаете еще папку facades? А сервисы поместите в корень в services? А если кодовая база начнет расти? Может вся логика приложения могла бы уйти в папку app как у Laravel?
Да, можно поместить в app, можно в src. У меня было в папке src, для статьи вынес это всё в корень для наглядности. Но если так подумать нет особой разницы где кодовая база растёт, в корне или в папке app :)
как-то неочевидно константа PROD в глобальном пространстве. Удобнее вынести в env конфиг
Да, я тут получается наметил схематично вариант, но не стал его описывать, хотя изначально хотел рассказать и включил файлик .env-example.
Чтобы считать из него переменные нужно использовать библиотеку Dotenv (https://github.com/vlucas/phpdotenv) и тогда
/*
.env-example
APP_ENV=1
*/
// web/index.php
$dotenv = Dotenv::createImmutable(__DIR__ . '/../');
$dotenv->safeLoad();
// config/routes.php
const PROD = getenv('APP_ENV');Можно разделить Router на App, Kernel и Route. Тогда в файле index.php будет создаваться инстанс App, который будет подгружать ядро, которое будет реализовывать логику роутера.
Да, это мне бы хотелось сделать, но только при наличии контейнера зависимостей.
Но с этим я пока не разобрался.
Не знаком с симфони, хотя я заюзал доктрину для моделек БД, и хочу это в другой статье описать, ну и make симфонивский заодно попробовать может тоже пригодиться :)
Простое REST api для сайта на php хостинге