Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert functions to static class methods. #59

Closed
stanvass opened this issue Jun 30, 2015 · 5 comments · Fixed by #274
Closed

Convert functions to static class methods. #59

stanvass opened this issue Jun 30, 2015 · 5 comments · Fixed by #274
Milestone

Comments

@stanvass
Copy link

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?

@sndsgd
Copy link

sndsgd commented Jul 1, 2015

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.

@stanvass
Copy link
Author

stanvass commented Jul 1, 2015

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.

@nikic
Copy link
Owner

nikic commented Jul 4, 2015

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.

@Potherca Potherca mentioned this issue Mar 13, 2016
@Potherca
Copy link

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?

@lcobucci lcobucci linked a pull request Feb 6, 2024 that will close this issue
@lcobucci
Copy link
Collaborator

lcobucci commented Feb 6, 2024

Handled in #274

Thank you!

@lcobucci lcobucci closed this as completed Feb 6, 2024
@lcobucci lcobucci added this to the 2.0.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants