feat: add the ability to add custom regex to route params

[vsergiu93]

This PR adds the possibility to add custom regex constrains on dynamic route segments

Syntax:

/blog/{id:\d{1,9}}
/blog/{category}/{type:article|news}

This PR only adds the capability to add custom regex, but there is no developer feedback, I mean at some point we should add some validation on this custom regexes so that we add some DX value, but I think this could be added as future improvements.

[coveralls]

Pull Request Test Coverage Report for Build 11214417817

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
• 44 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.2%) to 81.921%

---

Files with Coverage Reduction	New Missed Lines	%
src/Tempest/Container/src/Exceptions/InvalidInitializerException.php	4	0.0%
src/Tempest/Support/src/ArrayHelper.php	7	94.17%
src/Tempest/Container/src/GenericContainer.php	13	93.84%
src/Tempest/Support/src/StringHelper.php	20	89.06%

Totals
Change from base Build 11197684954:	0.2%
Covered Lines:	6430
Relevant Lines:	7849

---

💛 - Coveralls

[vsergiu93]

@brendt I feel like I should add more test cases but not sure where, I'm not sure if I'm missing some edge cases or not.

[aidan-casey]

@vsergiu93 - I have not read through the code here yet, however, I'm curious... Have you benchmarked this at all?

[vsergiu93]

Hey @aidan-casey, no, I guess I should right? If I'm not mistaken there is some repo already prepared with lots of controllers

[aidan-casey]

Yeah. There is one here. I personally haven't checked in on it for a while, so while I think it is up-to-date, not sure. 🙂

@brendt - I wonder if we could update CI to automatically test benchmarks.

[vsergiu93]

I think doing some benchmarks in the CI could be great.

[vsergiu93]

@aidan-casey I'll try to look in to running some benchmarks tomorrow morning

[vsergiu93]

I tried to setup the benchmark repo locally, after I manage to configure composer to use my branch for tempest/framework, I ran ./tempest serve and I got the following error preg_match(): Compilation failed: regular expression is too large at offset 290350. I tried to set pcre.backtrack_limit and I still got the error.

[vsergiu93]

I managed to run some benchmarks:

Setup

I cloned the tempest-benchmark repo and did some changes to setup/setup.php file, I'm generating only 100 items per type (this is because I was getting that error above with the original number) and I also added an return new Ok('test'); response to the templates (where applicable)

For the test itself I used the example code from here #175 and modified it to make it work.

<?php

declare(strict_types=1);

namespace App\Bench;

use PhpBench\Attributes\Revs;
use Tempest\Clock\Clock;
use Tempest\Clock\GenericClock;
use Tempest\Console\Testing\ConsoleTester;
use Tempest\Container\Container;
use Tempest\Container\GenericContainer;
use Tempest\Core\AppConfig;
use Tempest\Core\Kernel;
use Tempest\Core\Tempest;
use Tempest\Framework\Testing\Http\HttpRouterTester;
use Tempest\Http\GenericRequest;
use Tempest\Http\GenericRouter;
use Tempest\Http\Mappers\RequestToPsrRequestMapper;
use Tempest\Http\Method;
use Tempest\Http\Request;
use Tempest\Http\RouteConfig;
use Tempest\Http\RouteDiscovery;

use Tempest\Http\Router;

use function Tempest\map;

class GenericRouterBench
{
    protected string $root;

    protected bool $discoveryCache = false;

    /** @var \Tempest\Core\DiscoveryLocation[] */
    protected array $discoveryLocations = [];

    protected AppConfig $appConfig;

    protected Kernel $kernel;

    protected Container $container;

    protected ConsoleTester $console;

    protected HttpRouterTester $http;

    protected GenericRequest $genericRequest;

    public function __construct()
    {
        $this->root ??= __DIR__ . '/../../';

        $this->kernel ??= new Kernel(
            root: $this->root,
            discoveryLocations: $this->discoveryLocations,
            discoveryCache: $this->discoveryCache,
        );

        $this->container = $this->kernel->container;

        $this->console = $this->container->get(ConsoleTester::class);
        $this->http = $this->container->get(HttpRouterTester::class);

        // first static route
        $this->genericRequest = new GenericRequest(Method::GET, '/admin/controller_001', []);
        // last static route
        // $this->genericRequest = new GenericRequest(Method::GET, '/users/controller_100', []);
        // first dynamic route
        //$this->genericRequest = new GenericRequest(Method::GET, '/books/xyz', []);
        // last dynamic route
        //$this->genericRequest = new GenericRequest(Method::GET, '/test/a/b', []);

        $this->container->singleton(Request::class, fn () => $this->genericRequest);
        $this->container ->singleton(GenericRequest::class, fn () => $this->genericRequest);
    }

    #[Revs(10_000)]
    public function benchDispatch(): void
    {
        //$request = map($this->genericRequest)->with(RequestToPsrRequestMapper::class);
        /** @var Router $router */
        $router = $this->container->get(Router::class);

        $router->dispatch($this->genericRequest);
    }
}

Before my changes:

1. First static route
   

After my changes:

1. First static route
   

@aidan-casey Let me know what you think, I have to be honest, I'm a bit lost here, any ideas on specific test cases?

[brendt]

I actually think preg_match(): Compilation failed: regular expression is too large at offset 290350 is a pretty significant issue. My guess is you're running into it because there's now more regex added to the routes.

I do want to look into it, but right now I have other things higher on my priority list :/

[vsergiu93]

I have to mention that I was running in to that when generating 1k items per type, so that is about 3k controllers which each have like 4 or 5 routes (I'm talking here of the setup code that exists in tempest-benchmark). For sure allowing custom regex per params could increase the big regex size but that is only if you have routes with custom regex otherwise the code will jut put what we had before for each segment and I haven't really modified the templates to use custom regex params.

I think this issue theoretically exists because of the approach taken here on routing and generating one big regex, but, at the same time I think that there is a small chance someone has build or plans to build a project containing around 4-5k routes (the biggest that I worked on was 600+ routes built with laravel).

@brendt I think the issue regex being to large is not connected to my changes, or better put, is not an issue that was introduced with this changes, the issue is there either way, it may be that with my changes you need fewer routes to hit it but you will hit it either way in the end.
I think a separate issue should be created with this and research some possible solutions to chunk the big regex or something similar.

Btw, if you want we can put this back to draft or feel free to close it if you want.

[aidan-casey]

@brendt / @vsergiu93 personally I think this is worth continuing to look into. I'll have some time later today to look at this more in-depth.

Thanks for all your work on this so far!

[aidan-casey]

@vsergiu93 - First off, good work on this PR and thank you for digging into this! You've inspired me to keep looking at the router before v1 as there are some more optimizations we still haven't put together here.

Without those other optimizations in place, I'm not sure we will be able to improve performance too much. Overall, this looks pretty good. Just left a few comments.

Out of curiosity, do you know where the decrease in performance from your benchmarks is coming from?

[aidan-casey]

@brendt I think the issue regex being to large is not connected to my changes, or better put, is not an issue that was introduced with this changes, the issue is there either way, it may be that with my changes you need fewer routes to hit it but you will hit it either way in the end.

Side note, you are completely correct here, @vsergiu93. We were supposed to revisit the router to implement chunking, grouping of routes, and nested matching. None of that has happened yet, so I will open an issue for it to look into.

Because I want to.

[aidan-casey]

@vsergiu93 - I think this is good enough to merge. Let's address the discussion points in a follow-up issue / PR.