Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

Use TerminableInterface #15

Merged
merged 4 commits into from
Apr 16, 2015
Merged

Use TerminableInterface #15

merged 4 commits into from
Apr 16, 2015

Conversation

jeromemacias
Copy link
Contributor

Add UrlMap::terminate() method.

@jeromemacias
Copy link
Contributor Author

Waiting for stackphp/CallableHttpKernel#3 to add missing tests.

@davedevelopment
Copy link

Do we need to terminate them based on them being matched? I'm not sure, related to stackphp/builder#14

@jeromemacias
Copy link
Contributor Author

The goal of UrlMap stack is to manage only one matching app, so I think we should apply the same logic in handle() and terminate(), and ignore all non matching apps.

@davedevelopment
Copy link

But it's one matching app per request, the HttpCache middleware could and would fire multiple requests through the app before terminate happens.

@jeromemacias
Copy link
Contributor Author

Good point ;) I updated the PR accordingly.

@jeromemacias
Copy link
Contributor Author

I tested the UrlMap on a Silex project, with the WebProfiler provider which use the terminate kernel event. All seems good.

@davedevelopment
Copy link

Just thought of a really minor thing, perhaps we should keep track of the apps that have been terminated, something like:

$terminated = [];
foreach ($this->map as $path => $app) {
    if ($app instanceof TerminableInterface && !in_array($app, $terminated, true)) {
        $app->terminate($request, $response);
        $terminated[] = $app;
    }
}

Not sure if that would be overkill. I personally think terminate should be idempotent, so shouldn't matter.

@davedevelopment
Copy link

Other than that, let's :shipit:


public function terminate(Request $request, Response $response)
{
$pathInfo = rawurldecode($request->getPathInfo());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should no longer be needed

CHH added a commit that referenced this pull request Apr 16, 2015
@CHH CHH merged commit bfc9bcf into stackphp:master Apr 16, 2015
@CHH
Copy link
Member

CHH commented Apr 16, 2015

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants