Bug fix: Reset Route arguments for each call

[mathmarques]

Route arguments weren't reset for each call of App::process and if the route had optional arguments on the url this could cause unexpected behaviors.

This Fix #2430

[coveralls]

Coverage increased (+0.007%) to 97.088% when pulling 95585bd on mathmarques:fix-route-arguments into e84150b on slimphp:3.x.

[akrabat]

Why is process() called more than once for a given route?

[mathmarques]

@akrabat It's not more than once for a given route, but process can be invoked more than once for a single Slim's instance. And if one route is matched more than once, this problem will occur.
An use case: #1688

[fiskWasTaken]

This is indeed a problem you will run into if you try to use Slim under PPM/ReactPHP, so this would be appreciated

However spellcheck Proccess in your unit test method names, or consider shorter names (testOptionalArgTransience, testOptionalArgTransienceWithSetArguments)

[akrabat]

If I call setArguments(), then $savedArguments needs resetting I think?

[mathmarques]

I don't think so @akrabat .
After app initialization the only way to call setArguments() is after the route is matched, and I think if you use setArguments() at this time it's only for that one request.

[akrabat]

This test fails:

    public function testInvokeSequentialProccessAfterAddingAnotherRouteArgument()
    {
        $app = new App();
        $route = $app->get('/foo[/{bar}]', function ($req, $res, $args) {
            return $res->write(count($args));
        })->setArgument('baz', 'quux');

        // Prepare request and response objects
        $env = Environment::mock([
            'SCRIPT_NAME' => '/index.php',
            'REQUEST_URI' => '/foo/bar',
            'REQUEST_METHOD' => 'GET',
        ]);
        $uri = Uri::createFromEnvironment($env);
        $headers = Headers::createFromEnvironment($env);
        $cookies = [];
        $serverParams = $env->all();
        $body = new RequestBody();
        $req = new Request('GET', $uri, $headers, $cookies, $serverParams, $body);
        $res = new Response();

        // Invoke process with optional arg
        $resOut = $app->process($req, $res);

        $this->assertInstanceOf('\Psr\Http\Message\ResponseInterface', $resOut);
        $this->assertEquals('2', (string)$resOut->getBody());

        // Prepare request and response objects
        $env = Environment::mock([
            'SCRIPT_NAME' => '/index.php',
            'REQUEST_URI' => '/foo/bar',
            'REQUEST_METHOD' => 'GET',
        ]);
        $uri = Uri::createFromEnvironment($env);
        $headers = Headers::createFromEnvironment($env);
        $cookies = [];
        $serverParams = $env->all();
        $body = new RequestBody();
        $req = new Request('GET', $uri, $headers, $cookies, $serverParams, $body);
        $res = new Response();

        // add another argument
        $route->setArgument('one', '1');

        // Invoke process with optional arg
        $resOut2 = $app->process($req, $res);

        $this->assertInstanceOf('\Psr\Http\Message\ResponseInterface', $resOut2);
        $this->assertEquals('3', (string)$resOut2->getBody());
    }

Arguably, it's unlikely to happen, but this test works with 3.x today.

[mathmarques]

This change should fix it.
Couldn't think another way to solve it. Any better solution?

[akrabat]

Couldn't think another way to solve it. Any better solution?

Seems reasonable to me. I've renamed the variable to make it a bit clearer though.

[mathmarques]

@akrabat Should I patch this on 4.x? Or maybe raise an issue for review every change on 3.x tests since 4.x was created and add them to 4.x and fix it?

[akrabat]

@mathmarques Good point - this needs to go to 4.x too.

A proportion of them have been ported forward, but it'd be good to go through and check.