FastRoute icon indicating copy to clipboard operation
FastRoute copied to clipboard

Convert functions to static class methods.

Open stanvass opened this issue 9 years ago • 4 comments

Some applications use the router conditionally (say, depending on whether there's cache hit or miss before the router kicks in), and loading functions.php is always loaded which represents a slight performance loss (as these functions will never be called).

The code to load those conditionally means bypassing Composer's autoloading functionality and doing a require_once of functions.php manually, before using them.

If we convert those functions to static methods, their functionality will be identical, but they'll be able to be lazily loaded as any other class.

If I provide a patch for this, would you accept?

stanvass avatar Jun 30 '15 11:06 stanvass

The performance overhead for including a script of functions via a require statement is less than autoloading a class. That being said, you might be interested in nikic's view on functions. Even if you don't agree, he has written some great posts that are well worth reading.

sndsgd avatar Jul 01 '15 00:07 sndsgd

PHP is an interpreted environment. Even with opcode cache, loading code on every request that you don't use affects performance.

The performance overhead of including functions I won't use is not less than not loading a class I won't use. And loading a file with functions has identical performance to loading a class with static methods.

I have great respect for @nikic and I love reading his posts, but his premise is we use classes because someone told us it's fancier or "better".

First, we do it because functions in PHP were left behind by PHP itself. Classes got autoload, functions didn't. For several PHP releases functions were also excluded from name aliasing (use X as Y). In PHP 5.6 we got Foo::class for full name resolution from a short or aliased name. Functions? Nothing.

Second, @nikic uses Python as an example, but Python wouldn't reload its function definitions thousands of times per second like PHP does for every request. If PHP's architecture matched Python, we'd just eagerly preload entire libraries and it wouldn't matter.

Developers simply have to conform to the language's limitations, so they stick to primitives in the language that fit its architecture better.

stanvass avatar Jul 01 '15 17:07 stanvass

While I don't really buy the performance argument, I did experience that functions can cause weird autoloading issues and unless you're writing function-only libraries the hassle is often not worth it. As such I don't have a problem with using static methods for this in principle.

The main problem here is that converting the functions to static methods will break BC (unless you keep the functions around as well, in which case the point is kinda lost). I plan to do a BC-breaking 1.0 release sometime soon though (to change the format of the result array to use named keys), so we can include it there.

nikic avatar Jul 04 '15 10:07 nikic

I'm not sure how much this adds to this discussion, but it is becoming more and more common to put "factory" functions in static class methods.

There is, however, another option and I would like to know your opinions... Using the __invoke method, the following could just as easily be implemented:

<?php

namespace FastRoute;

class FastRoute
{
    final public function __invoke(callable $routeDefinitionCallback, array $options = [])
    {
        $options += [
            'routeParser' => 'FastRoute\\RouteParser\\Std',
            'dataGenerator' => 'FastRoute\\DataGenerator\\GroupCountBased',
            'dispatcher' => 'FastRoute\\Dispatcher\\GroupCountBased',
            'routeCollector' => 'FastRoute\\RouteCollector',
        ];

        /** @var RouteCollector $routeCollector */
        $routeCollector = new $options['routeCollector'](
            new $options['routeParser'], new $options['dataGenerator']
        );
        $routeDefinitionCallback($routeCollector);

        return new $options['dispatcher']($routeCollector->getData());
    }

    final public static function createSimpleDispatcher(callable $routeDefinitionCallback, array $options = []) {
        $fastRoute = new self;
        return $fastRoute($routeDefinitionCallback, $options);
    }
}

With something similar (CachedFastRoute?) for a cached version.

Besides the static method call this would also allow the following (to paraphrase the example from the README.md):

<?php

require '/path/to/vendor/autoload.php';

if ($shouldCache === true) {
    $fastRoute = new \FastRoute\CachedFastRoute();
} else {
    $fastRoute = new \FastRoute\FastRoute();
}

$dispatcher = $oFastRoute(function(RouteCollector $p_oRouteCollector) {
    $p_oRouteCollector->addRoute('GET', '/users', 'get_all_users_handler');
    // {id} must be a number (\d+)
    $p_oRouteCollector->addRoute('GET', '/user/{id:\d+}', 'get_user_handler');
    // The /{title} suffix is optional
    $p_oRouteCollector->addRoute('GET', '/articles/{id:\d+}[/{title}]', 'get_article_handler');
});

// ... 

Thoughts?

Potherca avatar Mar 13 '16 23:03 Potherca