Code Review ядра сайта?

Question

[Иван Антонов]

Есть тут хардкорные прогеры на PHP с 10+ летним стажем? Оцените, пожалуйста, ядро сайта. Мне нужна только критика. На сколько гибкое ядро, выдержит ли оно громадный проект? Не возникнет ли в будущем проблем из-за неправильной архитектуры.
Код почитать можно тут:
https://github.com/antonowano/site

Comments

[Антон Р.]

А что за код то? Для чего?

[Flying]

Думаю что такая задача в исполнении опытного разработчика будет стоить существенных денег. Опять же "громадный проект" - это какие порядки? Вы Facebook собираетесь на нём построить?

[Иван Антонов]

Flying, Задача код ривью? За это можно и заплатить. Но прежде нужно удостовериться в компетентности человека, который будет выполнять код ривью.

[Иван Антонов]

Антон Р., это каркас сайта, на котором можно будет построить любой сайт, используя пакеты composer.
Ядро состоит из:
Config - конфигурационный класс
Router - маршрутный класс
Factory - фабрика для строительства объектов часто используемых классов (базы данных, логеров, мейлеров)
Kernel - центральный класс

[Flying]

Иван Антонов, Я быстренько глянул ваш код. Пока что это выглядит как первые шаги эксперимента в попытке разобраться как вообще можно написать framework. Т.е. review там пока делать рановато т.к. сейчас там по факту ничего нет.

[Flying]

Видно определённое влияние того же Symfony, но пока что даже до версии 0.0.1 вам предстоит ещё долгий путь. Когда пройдёте его - тогда можно пробовать привлекать reviewer'ов

[Антон Р.]

У тебя роуты статичные?

<?php
return array(
    'routes' => array(
        '/' => 'Default:index',
        '/php' => 'Default:info',
    ),
    'route_404' => 'Default:notFound'
);

Я сделал подгрузку из БД у себя:

<?php

include ROOT . '/db/db.php';
$sql = 'SELECT * FROM routes';
$result = $pdo->query($sql);
while($r = $result->fetch())
	{
		$routes[]=array(
		'id'=>$r['id'],
		'uri'=>$r['uri'],
		'controller'=>$r['controller'],
		'action'=>$r['action']
		);
	}

return $routes;

?>

Сам роутер:

<?php

class Router {
	
	public $routes;
	
	public function __construct(){
		$this->routes = include(ROOT.'/routing/routes.php');
	}
	
	private function getURI(){
		if(!empty($_SERVER['REQUEST_URI'])){
			return trim($_SERVER['REQUEST_URI'], '/');
		}	
	}
	
	public function Run(){
		
		$uri = $this->getURI();
		foreach($this->routes as $route){
			
			if($route['uri'] === $uri){

				//Узнаем имя контроллера и экшена:
				$controllerName = $route['controller'];
				$actionName = $route['action'];
				
				//Назначаем имена файлу контроллера:
				$controllerFileName = $controllerName . 'Controller.php';
				
				//Подключаем файл контроллера:
				include ROOT . '/controllers/'.$controllerFileName;
				
				//Создаем класс контроллера:
				$controllerObject = new $controllerName;
				
				//Вызываем метод класса контроллера:
				$result = $controllerObject->$actionName();
				
				}
			}
		
	}
	
}

?>

[dmitriy]

Иван Антонов,
Ну слишком простой каркас. очень простой, минусы
- нет работы с responce request
- нет обработчиков ошибок
- нет нормального шаблонизатора
- render идет напрямую на вывод а если нужен шаблон письма с последующей отправкой?
- нет работы с консолью
- нет модульности
- нет работы с правами доступа

в вашем случае ядро прям слишком банальное и простое которое может написать любой студент на коленке

[Flying]

Антон Р., А зачем дёргать базу данных на каждый запрос? Ведь вы по сути на каждый запрос выполняете одинаковую операцию которую можно (и нужно) кэшировать. Кроме того ваше решение принципиально не поддерживает параметризированных запросов т.к. обработка $uri этого не содержит, а getURI() возвращает содержимое REQUEST_URI т.е. ставит крест на matching'е url'ей с query string. Попробуйте у себя сделать запрос на /book/1 или на /book?id=1

Иван Антонов в этом плане подошёл более ответственно, он хотя бы query string отрезает.

[Иван Антонов]

Flying, А поздно не будет? Меня в частности беспокоит не слишком ли жесткая связанность в ядре между Config, Router, Factory, Kernel.
Router, Factory имеют доступ к Kernel, для возможности взаимодействия разных компонентов. К примеру, если я захочу маршруты брать из БД, тогда я в Router'е через kernel могу получить доступ к Factory и подтянуть от туда соединение с БД. Мне кажется это слишком замудрённым.

[Антон Р.]

Flying,

А зачем дёргать базу данных на каждый запрос?

Что-бы через админку создавать пункты меню и назначать им типы контента (как в Joomla например).

Попробуйте у себя сделать запрос на /book/1 или на /book?id=1

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

[Иван Антонов]

dmitriy,
- по поводу request я решил не запариваться и работать с глобальными переменными. Если потребуется, я могу установить через компоузер request от symfony (или любой другой).
- обработчики ошибок мне не нужны, т.к. во время программирования я сразу буду видеть, где происходит ошибка и из-за чего. Пока что структура очень проста и не требует обработки. Если появятся узкие места, конечно я добавлю ексепшены
- шаблонизатор я принципиально не использую, php и так нормальный шаблонизатор.
- с render согласен. Ер мне достаточно написать один метод renderBuf который будет сохранять в переменную, а потом я могу спокойно отправлять письмом.
- работа с консолью можно построить через GET. или таким же макаром создать контроллер который будет отвечать чисто на консольные команды. Это изи.
- для модульности стоит компоузер, можно поставить всё что угодно. Можно в проекте сделать папку со своими компонентами.
- минус о работе с правами доступа вообще не понял. один раз настроил сервер, и больше не паришься.

в вашем случае ядро прям слишком банальное и простое которое может написать любой студент на коленке

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

[Flying]

Иван Антонов,

Сейчас (помимо целой кучи принципиально отсутствующих частей кода) у вас есть две большие проблемы:

1. Ваш код жёстко связан между собой.
   
2. Ваш код не разделяет код ядра и код пользователя
   

Для решения первой проблемы необходимо использовать интерфейсы. Выделение интерфейсов позволит сделать компоненты заменяемыми, хотя и заставит вас принять решение о том как же именно выбирать конкретный класс для того или иного интерфейса что постепенно приведёт вас к какой-либо из реализаций DI контейнера. Подумайте над тем какие методы вы ожидаете от того или иного сервиса в вашем коде, выделите их в интерфейсы и напишите реализации по-умолчанию. Не забывайте что PHP поддерживает не только прямое наследование для расширения (через extends), но и горизонтальное (через traits), это позволяет сделать код ещё гибче.

Также не пренебрегайте разделением функционально разного кода в разные namespaces, сейчас у вас всё лежит в одной куче.

Для решения второй проблемы вам необходимо задуматься об организации вашего кода. Посмотрите на "взрослые" framework'и и то как они жёстко разделяют код своего ядра от кода пользователя. У вас же сейчас весь код лежит в одной куче что требует от пользователя вашего кода либо жёсткой дисциплины в понимании своего / чужого кода либо ставит перед необходимостью самому заниматься раскладыванием кода в предложенные места. Подумайте как в вашем текущем варианте можно обновить код ядра не затрагивая код самого сайта? Пока что ответ находится где-то недалеко от "никак" и это очень плохо.

Но в целом, как я уже писал выше, сама идея попробовать написать что-то своё и разобраться - весьма полезна, получите хороший опыт и кучу шишек :)

[Flying]

Антон Р., Я понимаю идею стоящую за этим. То на что я пытался указать - это то что вы не кэшируете эти результаты. Согласитесь что конфигурация роутинга на сайте меняется заведомо реже чем идут запросы на сайт. Таким образом получается что если на X изменений роутинга у вас приходится Y запросов то Y/X запросов к базе данных заведомо лишние поскольку дают идентичные результаты. При этом, уверен, вы понимаете что X будет иметь значение порядка десятков, а Y может быть многотысячным.

Тем более что в данной задаче (вспоминаем Карлтона) критерий инвалидации кэша тривиальный.

[Иван Антонов]

Антон Р., да в будущем я планирую создать таблицу с маршрутами и с meta данными, но это уже в рабочем проекте. А тут пример со статичными, потому что статичные маршруты будут в любом проекте.

[Flying]

Иван Антонов, Зря вы так про Request и Response, ведь недаром же PSR-7 существует. Вообще для вас было бы прекрасным упражнением попробовать реализовать в вашем ядре поддержку всех имеющихся PSR, тогда действительно вы получите гибкий и расширяемый код. Попробуйте рассмотреть свою задачу в этом контексте.

[Иван Антонов]

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

[Иван Антонов]

Flying, Спасибо. Изучу PSRы.

[Flying]

Иван Антонов, 60к маршрутов легко сводятся к паре десятков в случае использования параметризированных роутов, вам не стоит забывать о них.

[dmitriy]

Иван Антонов,
разбираем

- по поводу request я решил не запариваться и работать с глобальными переменными. Если потребуется, я могу установить через компоузер request от symfony (или любой другой).

это не просто request это связка router + request + middleware

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

а на боевом? обработчик ошибок позволяет их гасить на боевом и сваливать в лог, у разраба выводить на экран в красивом виде

- шаблонизатор я принципиально не использую, php и так нормальный шаблонизатор.

php отвратительный шаблонизатор так как позволяет писать код в шаблонах не относящийся к view, также бывают различные задачи связанные с переопределением шаблонов, реализации блоков вывода и тд.

- работа с консолью можно построить через GET. или таким же макаром создать контроллер который будет отвечать чисто на консольные команды. Это изи.

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

- для модульности стоит компоузер, можно поставить всё что угодно. Можно в проекте сделать папку со своими компонентами.

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

- минус о работе с правами доступа вообще не понял. один раз настроил сервер, и больше не паришься.

имею ввиду Управление доступом на основе ролей

[Иван Антонов]

Flying, Да, я знаю о них из Symfony. Но очень часто ставятся задачи реализовать одноуровневый роутер.
/{alias} когда легче всего было бы сделать роутер - /company/{alias}

[Антон Р.]

Иван Антонов, первое что пришло в голову в плане оптимизации:

<?php

include ROOT . '/db/db.php';
$sql = 'SELECT * FROM routes WHERE uri = '.$uri;
$result = $pdo->query($sql);
while($r = $result->fetch())
  {
    $routes[]=array(
    'id'=>$r['id'],
    'uri'=>$r['uri'],
    'controller'=>$r['controller'],
    'action'=>$r['action']
    );
  }

return $routes;

?>

То есть выбирать только ту строку которая соотв. запросу. Можно заодно от foreach избавиться. Ну и индексировать этот столбец в БД.

[Flying]

Иван Антонов, В той же Symfony это легко решается через собственные реализации UrlMatcherInterface и UrlGeneratorInterface, внутрь которых, в качестве fallback'а можно пробросить matcher и generator, создаваемые Symfony по-умолчанию.

[Flying]

Антон Р., Какой восхитительный пример кода, открытого всем возможным SQL injections....

[Антон Р.]

Flying, ->bindvalue, знаем )

[Иван Антонов]

dmitriy, с middleware еще предстоит познакомиться в деталях. В популярных системах он довольно прозрачно работает.
Спасибо, посмотрю.

Логирования ошибок апача или nginx? Я для сложной логики можно будет подгрузить любой Logger.

Я не планировал в PHP шаблонах выстраивать какую-то сложную логику) Ну а блоки можно обычным инклудом делать. Единственное переопределение шаблонов тяжело реализовать (хоть и можно свой класс View реализовать).

Работа с консолью, тут я согласен. К примеру сложно будет реализовать вопрос-ответ в консоле.

По модульности всё равно не понял. Достаточно же писать функциональные части независимыми. В ядре на данный момент имеется только 4 класса, которые сильно связаны из-за того решаемых задач.

Управление доступом на основе ролей нету, с этим согласен, нужно будет реализовать.

Спасибо за разъяснение. Я теперь примерно понял, что мне нужно лучше изучить. А именно: PSR-7, PSR-3, Middleware, PHP-CLI, способы реализации ACL.

[Иван Антонов]

Антон Р., но не получится найти маршрут, если он у вас параметризированный.

[kafkiansky]

Антон Р., роуты из базы - это жестко. Больше не показывайте это никому.

[nrgian]

выдержит ли оно громадный проект?

громадность в чем меряете?

[lloydbanks]

Alex Wells, ваш коммент слишком хорош для коммента. Ему место в ответе среди лучших :)

[Дмитрий]

Иван Антонов, натравите на код любой статический анализатор кода
phpstan, exakat и тп

кодревью человека это не заменит, но некоторые мелочи, что человек мог пропустить, показать может

Answer 1

[Евгений Матвеев]

а что тут смотреть? кода почти нет, 99% кода в используемом фреймворке

Comments

[kafkiansky]

Где там фреймворк?

[Евгений Матвеев]

просто сделано похоже? в любом случае кода немного. если интересно поискать узкие места в плане нагрузки, попробуйте использовать webgrind-master