klein / klein.php

A fast & flexible router
MIT License
2.66k stars 290 forks source link

Lazy services with parameters #297

Closed dan1elhughes closed 9 years ago

dan1elhughes commented 9 years ago

I get Missing argument 1 for {closure}() when I attempt the following:

$router = new \Klein\Klein();

// Register services
$router->respond(function ($request, $response, $service, $app) {
    $app->register('hello', function($world) {
        return "hello $world";
    });
});

// ... Some time later
$router->get('/', function($request, $response, $service, $app) {
    echo $app->hello('world');
});

It seems that the issue is here, as the closure is called with no parameters. I'm currently not familiar enough with the code base to attempt a fix.

nbish11 commented 9 years ago

Short answer, it can be done. Long answer, it can't be done as it will most likely break compatibility.

The problem is the closure you registered will always return the same instance of Closure - like a singleton. You could in fact modify the register function in the App class to accept arguments, like so:

public function register($name, $closure)
{
    if (isset($this->services[$name])) {
        throw new DuplicateServiceException('A service is already registered under '. $name);
    }

    $this->services[$name] = function () use ($closure) {
        static $instance;
        if (null === $instance) {
            $instance = call_user_func_array($closure, func_get_args());
        }
        return $instance;
    };
} 

However, because of the line static $instance it will always be called with the same args:

$app = new \Klein\App;

$app->register('hello', function($world) {
    return "hello $world";
});

echo $app->hello('world');  // returns "hello world"
echo $app->hello('nathan');  // also returns "hello world"

If you would still like to register services without returning the same closure instance, Modify the register method in Klein's App class like so:

public function register($name, $closure)
{
    if (isset($this->services[$name])) {
        throw new DuplicateServiceException('A service is already registered under '. $name);
    }

    $this->services[$name] = function () use ($closure) {
        return call_user_func_array($closure, func_get_args());
    };
}

Now when you call the register method it will work as expected:

$app = new \Klein\App;

$app->register('hello', function($world) {
    return "hello $world";
});

echo $app->hello('world');  // returns "hello world"
echo $app->hello('Nathan');  // returns "hello Nathan"

I hope I have explained this clearly enough.

dan1elhughes commented 9 years ago

I see - very well explained! Looks like major changes would be required to add it.

For anyone looking with the same problem, I've worked around it by registering each service as returning a function, and then I can do the following:

$app->register('hello', function() { 
        return function($world) { 
                return "hello $world";
        }
}

$hello = $app->hello;
echo $hello('world);

While probably less efficient, this seems to do the job. Thanks!

nbish11 commented 9 years ago

:+1: Nice workaround. I didn't even think about returning a function.