-
Notifications
You must be signed in to change notification settings - Fork 444
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
Comments
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. |
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. |
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. |
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 <?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 ( 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? |
Handled in #274 Thank you! |
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?
The text was updated successfully, but these errors were encountered: