Router: fixes and questions

WinterSilence commented 5 years ago

https://github.com/sunrise-php/http-router/blob/85837a0ce1fb82fa937ca90560b511a4b2192665/src/RouteCollection.php#L148 $methods = (new \ReflectionClass(RequestMethodInterface::class))->getConstants();
Why route_regex excluded from Route?
Route::buildRegex() not chached compiled pattern - it's freeze executing
Route::group() cant defines attributes in prefix + cant set methods by default for group routes.
https://github.com/sunrise-php/http-router/blob/85837a0ce1fb82fa937ca90560b511a4b2192665/src/Route.php#L213 Why route cloned? This attributes not contains in RouteCollection. Class doesnt have other methods to set attributes + cant define attributes by default
https://github.com/sunrise-php/http-router/blob/85837a0ce1fb82fa937ca90560b511a4b2192665/functions/route_regex.php#L28 '/{(\w+)}/' - \w contains locale symbols, pure - '/{([a-z0-9_]+)}/'
'$#ui' URI attributes may be case sensitive, i vote to '$#uD'
Path prefix/suffix not contains separately - setPath() rewrite all path
https://github.com/sunrise-php/http-router/blob/85837a0ce1fb82fa937ca90560b511a4b2192665/functions/route_regex.php#L24 add slashes in setters (addSuffix, addPrefix, setPath)
https://github.com/sunrise-php/http-router/blob/85837a0ce1fb82fa937ca90560b511a4b2192665/src/RouteCollection.php#L37 id property dont used as key part - cant overwrite route dublicates, may be set $routes as SplObjectStorage? route - key, id - value;
https://github.com/sunrise-php/http-router/blob/85837a0ce1fb82fa937ca90560b511a4b2192665/src/Router.php#L103 in_array($request->getMethod(), $route->getMethods()) faster then preg_match($regex, $request->getUri()->getPath(), $matches). move up in_array
Why RequestHandler::handle() call only one middleware? why $middlewareQueue is SplQueue? It`s prevents using them again for inner(server side) requests
Cant find Route::getUri(array $attributes = []) : string or something like this - generate URI by route with attributes

fenric commented 4 years ago

In the coming days, version 2.0 will be released.

After the release, I will think about it.

https://github.com/sunrise-php/http-router/tree/release/v2.0.0

WinterSilence commented 4 years ago

@fenric

sometimes strange solutions, for example https://github.com/sunrise-php/http-router/blob/release/v2.0.0/src/RouteCollection.php#L93 why not:
```
public function addMiddlewares(MiddlewareInterface ...$middlewares): self
{
$this->middlewares = array_merge($this->middlewares, $middleware);
return $this;
}
```
https://github.com/sunrise-php/http-router/blob/release/v2.0.0/src/RouteCollection.php#L127 you remove last '/' in prefix, but not check it in path: $path = $this->prefix . ($this->prefix ? '/' . ltrim($path, '/') : $path);

Method purge not in RFC and unuseful for router.

https://github.com/sunrise-php/http-router/blob/release/v2.0.0/src/Route.php#L172 $this->methods = array_map('strtoupper', $methods);

https://github.com/sunrise-php/http-router/blob/release/v2.0.0/src/Router.php#L76

$routes = [];

$method = $request->getMethod();
foreach ($this->getRoutes() as $route) {
   if (in_array($method, $route->getMethods()) {
       $routes[] = $route;
   }
}

if (! $routes) {
    throw new MethodNotAllowedException($request);
}

$path = $request->getUri()->getPath();
foreach ($routes as $route) {
    if (path_match($route->getPath(), $path, $attributes)) {
        return $route->withAddedAttributes($attributes);
    }
}
throw new RouteNotFoundException($request);

Add Route::__toString() {return $this->name;} and calls as key in collection.
Cant findRouteCollection::group()`

90% routes pass $methods as ['POST', 'GET'] - set it as default value and set $attributes before $methods + $requestHandler not used and I dont think it's useful: Router:

public function route(
string $name,
string $path,
array $attributes = [],
array $methods = ['GET', 'POST'],
array $middlewares = []
)

Route:

public function __construct(
string $name,
string $path,
array $attributes = [],
array $methods = ['GET', 'POST'],
array $middlewares = [],
RouteInterface $parent = null
)

WinterSilence commented 4 years ago

@fenric Router::group() add new "parent" route with group routes as children:

In Route add methods getParent(): RouteInterface, withParent(RouteInterface $parent): self, getChildren(): array, withChildren(RouteInterface ...$children): self, withAddedChildren(RouteInterface ...$children): self, getFullPath(): string, getFullName(): string - path/name with parents prefix.

RouteCollection::$routes, Route::$children is RouteCollectionIterator extends RecursiveArrayIterator. RouteCollectionIterator::dispath(string $path): ?RouteInterface Recursive finds group/parent path in request path - if path included, crops parent path and find "tail" path in childs. Adds parent's attributes to attributes of latest route in chain.

'shop', '/shop' => [
 '/' or '' - 1 level route: name - 'shop', path -  '/shop' or  '/shop/'
'catalog', '/catalog' => [
   '/' or '' - 2 level route: name - 'shop/catalog', path -  '/shop/catalog' or  '/shop/catalog/'
   'category', '/category/{category}' => 3 level route: name - 'shop/catalog/category', path -  '/shop/catalog/%category_name%'/'
]
]

WinterSilence commented 4 years ago

@fenric Add Route::getUri(array $attributes = [], UriInterface $uri = null) : UriInterface

fenric commented 4 years ago

@fenric Add Route::getUri(array $attributes = [], UriInterface $uri = null) : UriInterface

https://github.com/sunrise-php/http-router/issues/27#issuecomment-553382909

WinterSilence commented 4 years ago

@fenric Route::getUri(array $attributes = [], UriInterface $uri = null) : UriInterface more flexible, your methods only returns path, you cant generates URI without UriInterface instance.

fenric commented 4 years ago

@WinterSilence

in 2.0 version, RouteCollection::any() method is missing. and ReflectionClass isn't fast.
in 2.0 version, Route::buildRegex() method is missing. and a route shouldn't know this.
see above.
grouping in 2.0 version:

$router = new Router();

$group1 = new RouteCollection();
$group1->setPrefix('/group1');
$group1->addMiddlewares(...);
$group1->get('foo1', '/foo1', ...);
$group1->get('foo2', '/foo2', ...);

$group2 = new RouteCollection();
$group2->setPrefix('/group2');
$group2->addMiddlewares(...);
$group2->get('foo3', '/foo3', ...);
$group2->get('foo4', '/foo4', ...);

$router->addRoutes(
    ...$group1->getRoutes(),
    ...$group2->getRoutes()
);

if you have a long-running application, you will have problems. you can't change the state of a route.
yes, I see... you're right. https://www.php.net/manual/ru/regexp.reference.escape.php
you're right. it's in release 2.0.
it problem is not in version 2.0.
it problem is not in version 2.0.
I'm still thinking about it ...
it problem is not in version 2.0.
it problem is not in version 2.0.
help me please: https://github.com/sunrise-php/http-router/issues/27#issuecomment-553382909

fenric commented 4 years ago

@WinterSilence

sometimes strange solutions: it's not strange solutions :) array_merge isn't fast, foreach will be faster...
you remove last '/' in prefix...: the path should always start with a slash. why set the path without a slash in the beginning?
yes, you're right. but, in HTTP API the verb PURGE very popular. for example, to delete a cache.
no, array_map isn't fast, foreach will be faster...
Can't find RouteCollection::group()...: I wrote above that the grouping in version 2.0 is now different.
Add Route::__toString()...: I'm still thinking about it ...
90% routes pass... oh no, this leads to uncertainty. 1 route 1 action. https://www.restapitutorial.com/lessons/httpmethods.html (yes, it's for HTTP API, but still).

P.S. would you like it to be not Route::withAttributes() but Route::withAddedAttributes()?

fenric commented 4 years ago

@fenric Add Route::getUri(array $attributes = [], UriInterface $uri = null) : UriInterface

It looks beautiful, but what's the point? because we only have an URI element path...

WinterSilence commented 4 years ago

@fenric

in 2.0 you can add const METHODS = [self::METHOD_GET, ...]
how add sub-group?
I check 2.0 - it's still here

WinterSilence commented 4 years ago

@fenric

doesn’t matter in this case
see PSR-7 UriInterface
doesn’t matter in this case
it's only default value

P.S. would you like it to be not Route::withAttributes() but Route::withAddedAttributes()?

it's similar to PSR-7 - method not replace all current attributes to new.

WinterSilence commented 4 years ago

@fenric

because we only have an URI element path

this method change path and return UriInterface $uri

WinterSilence commented 4 years ago

@fenric maybe adds setDefaultAttributes(), getDefaultAttributes() for set/get attribute values by default?

WinterSilence commented 4 years ago

@fenric

foreach will be faster

if you stay on this way why path_regex not use it? here it makes more sense than anywhere else

fenric commented 4 years ago

@WinterSilence It's hard to add a PSR-7 URI. I know what is it: https://github.com/sunrise-php/uri

We will have a similar situation: https://github.com/sunrise-php/http-router/issues/23

WinterSilence commented 4 years ago

@fenric многа букаф, в общих чертах в чем проблема?

fenric commented 4 years ago

@fenric многа букаф, в общих чертах в чем проблема?

о, вы можете писать по русски, как же я рад... проблема в следующем: если Route::buildUri() будет возвращать PSR-7 Uri объект, то следовательно, необходимо что-то типа этого: https://github.com/sunrise-php/uri

зашивать еще одну зависимость в маршрутизатор, наверное плохая идея, однако можно решать проблему, скажем так:

$route->buildUri(new UriFactory(), array $attributes, bool $strict)

выглядет красиво, но удобно ли это использовать? фабрики это уже PSR-17, не многие ее поддерживают, взять тот же Slim, там нет PSR-17.

поэтому, вся эта идея довольно сложная, и я бы ограничился просто Route::buildPath().

WinterSilence commented 4 years ago

@fenric конечно говорю, но только в данном случае т.к. в обсуждении участвуем только мы. метод возвращает объект UriInterface, а не Uri т.ч. конкретная реализация UriInterface не важна и никаких новых зависимостей нет - роутер и так поддерживает PSR-7.

fenric commented 4 years ago

@WinterSilence а как мы сгенерируем Uri? мы должны вызвать new Uri(), как это сделать не внедрив зависимость? Только используя фабрику... Фабрика это PSR-17 (не 7), вызов метода усложнится:

$route->buildUri(new UriFactory(), array $attributes, bool $strict)

Ведь при желании, можно на стороне пользователя реализовать объект/хелпер, скажем так:

(new RouteHelper)->buildUri(RouteInterface $route, array $attributes, bool $strict)

... или что-то более изящное:

(string) new RouteUri(RouteInterface $route, array $attributes, bool $strict)

От сюда и сомнения, о необходимости оного...

WinterSilence commented 4 years ago

@fenric туплю, я изначально предполагал возвращать строку, поэтому $uri = null. т.ч. getUri(UriInterface $uri, array $attributes): UriInterface;

WinterSilence commented 4 years ago

@fenric можно и строку возвращать, но так удобнее т.к. возврается уже другой объект.

fenric commented 4 years ago

@WinterSilence '/{(\w+)}/' - \w contains locale symbols, pure - '/{([a-z0-9_]+)}/'

complete...

https://github.com/sunrise-php/http-router/blob/release/v2.0.0/functions/path_regex.php

WinterSilence commented 4 years ago

@fenric зря копался - https://github.com/sunrise-php/http-router/pull/31

fenric commented 4 years ago

@fenric зря копался - #31

Выглядит интересно, сегодня бенчмарк напишу, проверю, так как есть подозрение, что foreach + str_replace могут быть не быстрее preg_replace_callback... вообще я бы выкладывал бенчмарки, но выглядят они довольно грязно...

WinterSilence commented 4 years ago

@fenric явно быстрее чем дважды разбирать строку да еще и с генераций анонимных функций + результат более корректный, например, вместо [^/]+ передается [^/<>()]+

WinterSilence commented 4 years ago

@fenric я пытаюсь скрестить ваш роутер и наш т.к. наш морально устарел и не поддерживает группы. в принципе можешь подождать немного и я сам 2.0 напишу :)

fenric commented 4 years ago

@fenric явно быстрее чем дважды разбирать строку да еще и с генераций анонимных функций + результат более корректный, например, вместо [^/]+ передается [^/<>()]+

да, разница была, на 1.14 ваш вариант быстрее...

WinterSilence commented 4 years ago

@fenric тут и без тестов ясно что быстрее - лишний раз строка не парсится, но, как я уже сказал, различия не только в скорости.

WinterSilence commented 4 years ago

@fenric

no, array_map isn't fast, foreach will be faster

foreach + str_replace могут быть не быстрее preg_replace_callback

сам себе противоречишь кстати

WinterSilence commented 4 years ago

@fenric и раз уж никаких замыканий теперь нет, то не вижу причин не перенести path_regex в метод Route::buildPathRegex() , a #31 можно закрыть.

WinterSilence commented 4 years ago

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

fenric commented 4 years ago

@WinterSilence в общем, методы Route::buildPath() and Route::buildRegex() готовы...

https://github.com/sunrise-php/http-router/blob/release/v2.0.0/src/Route.php

на сколько я понимаю, из всего issue, остался один вопрос, это подгруппы для маршрутов...

на практике, в монолите, хотя я исключительно за микросервисную архитектуру, я вижу не так много групп:

/api/v1 - HTTP API приложения; /api/v* - новые версии HTTP API (2, 3, etc.) приложения; /admin - панель управления;

и в принципе все, остальное это foreground or public area так сказать... и то, по хорошему HTTP API должно быть на поддомене за gateway стоять, равно как и панель управления, также на поддомене, а в идеале вообще, локально в офисе. но это я отошел от сути... я вижу ваши примеры, мне просто важно понять, подгруппы действительно могут быть востребованы? просто к примеру, я бы не стал разбивать адрес целевого ресурса shop/catalog/category на 3 подгруппы, ввиду того, что процессы отладка/трассировка могут усложниться... с другой стороны, если такой подход уже используется, то возможно, стоит вернуть логику подобную group как минимум для обратной совместимости...

мне интересно ваше мнение на этот счет...

fenric commented 4 years ago

@fenric

no, array_map isn't fast, foreach will be faster

foreach + str_replace могут быть не быстрее preg_replace_callback

сам себе противоречишь кстати

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

fenric commented 4 years ago

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

я потерял нить повествования, был бы признателен за помощь в ее поиске ;) о какой функции идет речь, о чем спрашивать, и чего я не понимаю?

WinterSilence commented 4 years ago

@fenric

подгруппы для маршрутов

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

у меня сайт строится из приложений(frontend, backend) и используемых в них пакетов (shop, blog, forum). Контроллеры в пакетах могут быть сгруппированы по папкам/подпространствам имен.

Например, группа приложения содержит подгруппы пакетов которые могут содержать подгруппы контроллеров. shop / catalog / product / x - приложение / пакет / группа / контроллер, class Shop\Frontent\Catalog\Product backend / blog / post / list - приложение / пакет / контроллер, class Blog\Backend\Post

о какой функции идет речь

path_regex, какой смысл выносить эту функцию за пределы Route?

fenric commented 4 years ago

@WinterSilence у вас какой-то навороченный HMVC... так как у вас компонент, внутри которого другие компоненты, грубо говоря, разумеется. тогда действительно, имеет смысл вернуть аналог group(callable $callback)... этим я сейчас и занимаюсь... просто сама логика клонирования, и коллбек в коллбеке, меня сильно терзала, ввиду чего я и решил было отказаться от них.

я вынес 3 функции за пределы src, чтобы увеличить читаемость кода в целом, эти 3 функции самые "грязные". я их обозначил как хелперы, с практической точки зрения – нормально, подобное можно встретить в том же zend-diactoros... плюс эти функции можно использовать процедурно, не инициализируя объекты, так как я противник статических методов в классах, от чего и нет класса Helper или подобного. такова природа моего решения.

WinterSilence commented 4 years ago

@fenric Посмотри мой Route. По хорошему патч не должен содержать шаблоны аттрибутов <...>, тогда и парсить их не придется. Шаблоны передаются в конструктор роута вместе с дефолтными значениями атрибутов. + ленивая генерация regex на основе path.

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

В нашем случае функция используется только в одном месте т.ч. смысла в этом нет.

метод buildRegex() должен быть protected и его значение должно храниться в отдельном свойстве и генерироваться только при необходимости, тогда path_build вообще не будет нужен т.к. его можно будет заменить на str_replace.

WinterSilence commented 4 years ago

$route = new Route(
   'product', 
   '/admin/shop/product/<action>(/<id>)', 
    [
       'namespace' => 'Shop\Controller\Backend', // значение по умолчанию
       'controller' => ['Product'], // [значение по умолчанию]
       'action' => ['index', 'create|update|delete'], // [значение по умолчанию, шаблон]
       'id' => [0, '\d+']
    ],
    ['GET', 'POST']
)
// $route->buildPath(['action =>'create']);
// `Shop\Controller\Backend\Product::actionCreate()`

WinterSilence commented 4 years ago

@fenric

у вас какой-то навороченный HMVC

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

WinterSilence commented 4 years ago

@fenric да еще такой момент - если подключил что-то через use, то в классах-наследниках повторно их подключать не обязательно.

namespace Sunrise\Http\Router;
use Psr\Http\Server\MiddlewareInterface;
use Psr\Http\Server\RequestHandlerInterface;
interface RouteInterface

namespace Sunrise\Http\Router;
// use Psr\Http\Server\MiddlewareInterface;
// use Psr\Http\Server\RequestHandlerInterface;
use function strtoupper;
class Route implements RouteInterface

fenric commented 4 years ago

@fenric да еще такой момент - если подключил что-то через use, то в классах-наследниках повторно их подключать не обязательно.

оно там подключено не просто так ;)

public function setRequestHandler(RequestHandlerInterface $requestHandler)

public function setMiddlewares(MiddlewareInterface ...$middlewares)

по качеству кода, я на scrutinizer всегда пытаюсь добиться наивысшей оценки :) все по PSR2, по "феншую" так сказать :)

WinterSilence commented 4 years ago

@fenric

оно там подключено не просто так ;)

RequestHandlerInterface, MiddlewareInterface УЖЕ объявлены в RouteInterface, в Route их не нужно повторно подключать.

все по PSR2, по "феншую" так сказать :)

тогда у меня для тебя плохие новости - PSR-2 уже давно Deprecated

WinterSilence commented 4 years ago

@fenric

по качеству кода, я на scrutinizer всегда пытаюсь добиться наивысшей оценки

scrutinizer-ci.com? он чем то принципиально отличается от старичка travis'а? этим сервисом еще не пользовался. код у тебя хороший, это не ошибка, просто лишнюю работу делаешь

fenric commented 4 years ago

@WinterSilence вы ошибаетесь, если думаете, что однажды использовав use scope/classname, больше этого делать нигде не нужно... попробуйте сделать то, о чем вы меня просите, и убедитесь, что будет fatal error, так как при сборке, движок начнет искать классы:

Sunrise\Http\Router\MiddlewareInterface Sunrise\Http\Router\RequestHandlerInterface

потому, что каждый файл, ничего не знает о конструкции use в других файлах, равно как и declare например...

что касается scrutinizer, я его использую как статический анализатор кода, travis я использую для запуска тестов. так как в запуске тестов последний сильно быстрее первого. а вот travis не представляет статического анализатора кода, равно как и не предоставляет coverage по коду... это 2 разных проекта, но я использую оба...

на смену PSR2 пришел PSR12, последний включает в себя первый, он устарел в том смысле, что при разработке нужно ориентироваться на PSR12 сразу, а не на PSR2... PSR12 также как и PSR2 включает в себя PSR1... PSR1 вроде как тоже не устарел, просто сегодня его сильно расширили... я бы вообще выражался просто: все написано по PSR, и нет лишних вопросов :)

fenric commented 4 years ago

@WinterSilence

public function group(string $prefix, callable $callback) : void
{
    $children = new self;
    $children->setPrefix($this->prefix . $prefix);

    $callback($children);

    $this->addRoutes(...$children->getRoutes());
}

пожалуй, на данный момент, мне видится это не таким уж и плохим решением нашей проблемы...

минимум кода, следовательно проще поддерживать;
привычный синтаксис для пользователя, распространенный среди микро-фреймворков, с ходу могу привести Slim и Lumen;
обратная совместимость, это не мало важно, я и так устроил головную боль, сменив Middleware на RequestHandler для endpoint объектов... но это была вынужденная мера.

fenric commented 4 years ago

@WinterSilence для разбора/сборки путей написан парсер, теперь это работает в 1.5 раза быстрее регулярных выражений и вводит более строгие правила для написания таких путей...

благодаря новому парсеру стало возможным собирать пути с not required (optional) атрибутами... об этом здесь: https://github.com/sunrise-php/http-router/issues/28#issuecomment-554643163

добился качества кода 10/10 и 100%-ого покрытия кода тестами... и в целом не вижу теперь препятствий для релиза...

что думаешь на счет релиза?

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

WinterSilence commented 4 years ago

@fenric этой темой с use я сам себя в ступор вогнал: обслуживаем много серверов где нет возможности обновить php т.к. много чужого legaсy кода. Поэтому написали кое-какие "прекомпиляторы" кода для конвертации основных нововведений в legacy аналоги: $a ?: $b -> $a ? $a : $b. Ну и увлеклись немного... В итоге use'ы импортируются автоматом. И я серьезно не мог понять почему это у тебя не работает, даже тесты начал написал и пока писал вспомнил)))

что думаешь на счет релиза?

не знаю кто больше перемудрил ты или я, доделаю свой варинт - сравним. опциональность была и в нашем старом роутере + возможность задать значения по умолчанию -> даже отстутствующие опциональные атрибуты будут иметь значения: шаблон пути: /<controller>(/<action>) дефолтные значения: ['namespace' => 'abc', 'action' => 'index'] путь: /foo/bar или /foo/bar/ значения: ['namespace' => 'abc', 'controller' =>'foo', 'action' => 'bar'] короткий путь: /foo или /foo/ значения: ['namespace' => 'abc', 'controller' =>'foo', 'action' => 'index']

я делают вариант с вложенными роутами:

Группа - это роут реализующий доп. интерфейс, коллекции удалены за ненадобностью
Шаблоны для атрибутов передаются отдельно от шаблона пути т.ч. доп. парсинг пути не требуется, шаблон пути не содержит префикс группы - парсится в разы быстрее.
Запрос проверяется/передается по цепочке вложенных middleware/роутов. при этом при переходе на следующий уровен вложенности запрос клонируется - в него передаются полученные значения атрибутов из роута + из пути запроса удаляется "префикс" - путь роута.
Проверяем путь запроса: если пустой т.е. '' или '/' и есть роуты-наследники с обязательными атрибутами, то переходим к следующему роуту на этом уровне.
Если при выполнении middlewares возникает исключение, то переходим к выполнению middlewares следующего роута на этом уровне, если роут последний - на уровень выше.
Роутер содержит 2 группы middlewares: пре и пост обработка запроса.
Middlewares из группы пре обработки могут служить, например, для поиска имени роута по пути запроса в кеше.
Middlewares из группы пост обработки могут служить, например, для нормализации значений аттрибутов содержащих имена контроллеров и пространств имен: str_replace(['-', '_', '/'], '', ucwords($attr, '-_/')) или кешировать имена роутов и соответствующие им пути запросов для быстрого поиска.
Проверка пути и методов роута заданы в классе RoutingMiddleware он всегда идет первым в списке middlewares роута и передается на этапе его создания.
В случае успешной проверки RoutingMiddleware вызывает цепочку middlewares роута
Дальше вызов по цепочке переходит либо к первому роуту-наследнику(если роут - группа и есть наследники) либо к middlewares для пост обработки роутера.

В общем создаем цепочку вызовов middlewares:

роутер - пред обработка (на которой все может и завершиться если имя роута получено из кеша)
роуты - рекурсивый поиск (на котором тоже все может закончиться если роут не найдет)
роутер - пост обработка

Описание получилось не не особо толковым :) днем накидаю псевдокод чтобы было понятнее. На первый взгляд все довольно сложно, но на деле наоборот. для примера, получение полного ими роута состоящего из имен роутов разделенных точками:

public function getFullName(): string
{
    $prefix = $this->parent ? $this->parent->getFullName() . '.' : '';
    return $prefix . $this->name;
}

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

WinterSilence commented 4 years ago

@fenric еще раз посмотрел твой парсинг. еще один довод в пользу переноса подобного кода в класс - класс можно как минимум сериализовать и сохранить в файл чтобы каждый раз не генерировать "кеш". Ленивая генерация регулярок для путей + самое примитивное кеширование коллекции дадут тот же результат, а может и лучше, при этом код будет намного проще и меньше.
Кроме того думаю стоит добавить в роут флаг определяющий использовать ли кеширование для данного роута? если да, то кешировать результаты Route::buildPath() в переменную, а Route::buildRegex(), Router::hadler() в файл. Ссылки дублируются в разных блоках и генерировать их по 2-3 раза не есть гуд.

fenric commented 4 years ago

@WinterSilence я порой совсем не понимаю систему оповещений на github, поэтому, если ты принял приглашение, перейди пожалуйста сюда, и напротив своего аккаунта смени private на public...

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

мне сложно говорить про перемудрил, при разработки я придерживаюсь некоторых принципиальных постулатов, я стараюсь писать максимально чисто, максимально просто, максимально объектно-ориентировано, и не писать того, без чего можно обойтись... конечно озираясь на общепринятые практики, поскольку мы в open-source пространстве, иначе продукт не будет воспринят обществом. и если бы не это, возможно продукт был бы еще тоньше.
описанное в первом пункте, объясняет то, почему функции вынесены из классов, и почему некоторые из них могут отнестись к категории перемудрил. все эти функции содержат от части очень грязный код, но не просто так, изначально я позиционировал маршрутизатор как очень быстрый, и я очень рад тому, что я нашел в себе силы, отказаться от регулярных выражений и написать свой парсер, это увеличило скорость разбора путей в полтора раза. и наверное только парсер я отношу к категории перемудрил. верни я сейчас эти функции в классы, оценка кода сразу упадет, код станет грязным, я этого не хочу. я стараюсь держать баланс, в конце концов нужно понимать, что в процедурном программировании нет ничего плохого, от слова совсем. и нет, я не противоречу сам себе, когда говорил об объектно-ориентированном программирование, поскольку эти функции были написаны для работы продукта, а не для пользователя. 2.2. продолжая говорить о функциях, хочу отметить, что на каждую из них есть wrapper через объекты, следовательно, пользователь в принципе может ничего о них не знать. а если и знает, то только то, что они для внутренней работы продукта. можно представить, что их вообще нет, а все эти функции path_* поставляются самим php из коробки :) 2.3.1. я упомянул, что каждая функция имеет wrapper в объектах, следовательно при сериализации объекта, эффект будет тот-же, будь-то весь код функций в классах... 2.3.1.1 я упомянул про сериализацию, но что-бы все работало совсем так, как нужно, необходимо, чтобы результаты работ таких функций хранились в свойствах объектов, сейчас этого нет. у меня в планах реализация кеширования (которого возможно не будет), именно в эту работу будет входить сериализация, поэтому в данный момент я бы оставил все как есть. а для долгоиграющих приложений смысла в кешировании вообще нет, так как оно есть уже из коробки при запуске приложения, и для таких приложений смысла вообще нет в "файлах", "сериализации", "кешировании", все это в таких приложениях скажется наоборот негативно, нежели позитивно.

ни вы, ни я не перемудрили, у наших маршрутизаторов разная ниша, ваш отлично подойдет для построение сложной покомпонентной CMS, sunrise разрабатывался в первую очередь для более простых проектов, в первую очередь для разработки API, для long running application, и справляется со своей задачей отлично. конечно на нем можно тоже написать все, что угодно. но тем не менее.

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

sunrise тоже поддерживает значения по умолчанию:

$router->get('read', '/read(/{id<\d+>})', new ReadHandler(), [], [
    'id' => 1,
]);

поэтому можно запустить маршрут по ссылке /read, и в обработчике запроса будет атрибут id со значением 1.

передача шаблонов в маршрут вне пути... сейчас я думаю об этом, из соображений обратной совместимости и у меня впереди еще скрещивание с zend-expressive, но возможность задавать их напрямую в пути останется, это практика, это объективнее удобнее пользователю, и сильно удобнее при использовании аннотаций, а long running application with annotations я обожаю и только так и веду разработку. и очень надеюсь, что рано или поздно, это станет частью языка, сообществу это интересно, и это не может не радовать (как минимум меня): https://wiki.php.net/rfc/annotations https://wiki.php.net/rfc/annotations_v2

Ссылки дублируются в разных блоках и генерировать их по 2-3 раза не есть гуд.

тут я мог чего-то не понять... но скажу одно, что ничего по 2-3 раза в sunrise не вызывается, это факт. если мы говорим конечно о функциях path_*

WinterSilence commented 4 years ago

@fenric

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

это уже "фичи" самописного разбора регулярок и нестандартного синтаксиса - я использую стандартные именованные подмаски <attribute>

Также можно использовать именованные подмаски с помощью синтаксиса (?P<name>pattern). Эта подмаска будет индексирована в массиве совпадений кроме обычного числового индекса, еще и по имени name. В PHP 5.2.2 было добавлено два альтернативных синтаксиса: (?<name>pattern) и (?'name'pattern).

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

2.3.1. я упомянул, что каждая функция имеет wrapper в объектах, следовательно при сериализации объекта, эффект будет тот-же, будь-то весь код функций в классах...

public function buildRegex() : string
{
    return path_regex($this->path);
}

так это же не callback'и, как подобная обертка связана с сериализацией?

2.3.1.1 я упомянул про сериализацию, но что-бы все работало совсем так, как нужно, необходимо, чтобы результаты работ таких функций хранились в свойствах объектов,

тут я мог чего-то не понять... но скажу одно, что ничего по 2-3 раза в sunrise не вызывается, это факт. если мы говорим конечно о функциях path_*

я не про какую-то конкретную реализацию, я говорил в общем про шаблоны сайта. В sunrise я не нашел полной реализации MVC, а тут речь о View - ссылка может быть как в верхнем меню так и подвале или в боковом меню. Т.ч. роутер в нынешнем виде будет генерировать одну и ту же ссылку по несколько раз.

простое решение:

protected $buildedPaths = [];
public function buildPath(array $attributes = [], bool $strict = false): string
{
    $attributes += $this->attributes;
    $path = array_search($attributes, $this->buildedPaths);
    if (false === $path) {
        $path = path_build($this->path, $attributes, $strict);
        $this->buildedPaths[$path] = $attributes;
    }
    return $path;
}

sunrise-php / http-router

Router: fixes and questions #29