search

slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.98k stars 1.95k forks source link

Question on \Slim\Router->finalize() #1627

Closed mathmarques closed 8 years ago

mathmarques commented 8 years ago

Why Slim needs to call all Routes->finalize() via \Slim\Router->finalize()? I tried to look for a reason but I did not find.

My question is because I'm trying to work around this issue: https://github.com/slimphp/Slim/issues/1604

Why not 'finalize' only the dispatched Route?

codeguy commented 8 years ago

I think this is a fair assessment. I don't see why we have to finalize all routes instead of the one matching route. However, I don't think this is paramount for the 3.0.0 RTM release. Will move to the 3.1.0 milestone.

codeguy commented 8 years ago

There are some edge cases such as sub requests, etc. We'd want to save the "finalized" state as a member variable in each route instance, and then finalize routes lazily on invocation. But a problem to be addressed later in 3.1.0 for sure.

mathmarques commented 8 years ago

I thought about sub requests and I moved the finalized state to Route class on my "fix" for Lazy resolving of route middlewares: https://github.com/mathmarques/Slim/commit/99a85f9f1fbabfde730e160898da0024e56c7bb3

@codeguy Is it too early for create a PR for change this finalize behavior? :)

codeguy commented 8 years ago

Not too early. Just be prepared to potentially rebase your PR in the future.

codeguy commented 8 years ago

If this is running successfully, we may as well just merge this into RC3 /cc @akrabat

silentworks commented 8 years ago

@codeguy can we get this tested on an app that makes heavy use of middleware's first, both Route and App level middleware.

codeguy commented 8 years ago

@silentworks For sure.

mathmarques commented 8 years ago

I just need to work on removing the \Slim\Router->finalize() (A lot os unit tests use this function and I just remove the content of it)

I'll make the changes and open a PR.

akrabat commented 8 years ago

Fixed by #1632.