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

Prototype for allowing immutable RouterInterface #189

Closed
wants to merge 1 commit into from
Closed

Prototype for allowing immutable RouterInterface #189

wants to merge 1 commit into from

Conversation

DASPRiD
Copy link
Member

@DASPRiD DASPRiD commented Nov 17, 2015

This PR is aimed to make the RouterInterface completely immutable. The way to achieve this is by injecting a RouterFactoryInterface into the Application, instead of a RouterInterface. The factory allows the same addRoute() method as the prior mutable router did. When requested, a router is build using the supplied routes.

@@ -383,6 +392,10 @@ public function routeMiddleware(ServerRequestInterface $request, ResponseInterfa
*/
public function route($path, $middleware = null, array $methods = null, $name = null)
{
if (null !== $this->router) {
// @todo should we throw an DomainException here, as routes cannot be added after the router was created?
Copy link
Member Author

Choose a reason for hiding this comment

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

See here.

@danizord
Copy link
Contributor

@DASPRiD LGTM, but I think it could be named RouterBuilderInterface in order to avoid confusion with callable container factories and because it does creation step-by-step.

@weierophinney
Copy link
Member

I'm still wondering what benefit this provides. Since we still end up with one mutable object (the "builder"), what does this extra indirection provide tangibly to the system or the end user?

@DASPRiD
Copy link
Member Author

DASPRiD commented Nov 18, 2015

@weierophinney Once the router is created from the builder, it is save to assume that it is immutable then. The app doesn't call the builder twice.

@weierophinney
Copy link
Member

@DASPRiD My point is: the liklihood of adding routes after bootstrapping is ridiculously low, and we can instead specify in the current RouterInterface that addRoute() MUST raise an exception once either match() or generateUri() have been called. That's the only real benefit I'm seeing to your changes, but I'm not sure that change outweighs the complexity, particularly when there could be a simpler approach.

@DASPRiD
Copy link
Member Author

DASPRiD commented Nov 18, 2015

@weierophinney Well, if we kinda allow the router to freeze, that'd probably be fine as well I suppose.

@weierophinney
Copy link
Member

@DASPRiD We already have verbiage stating that addRoute() should aggregate only, but not pass the route information to the underlying until one of those two methods are called. I think we can and should stipulate that it should raise an exception if called after one of those is called as well, which gives the benefits you're proposing in here.

Do you want to do a PR for that? Or alter this one? or leave it to me?

weierophinney added a commit to weierophinney/zend-expressive that referenced this pull request Dec 1, 2015
Per zendframework#189, `addRoute()` **MUST** raise a `RuntimeException` if called
after either `match()` or `generateUri()` have been called, to ensure
the router state does not change between invocations.
@weierophinney
Copy link
Member

Closing in favor of #202.

weierophinney added a commit that referenced this pull request Dec 1, 2015
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.

3 participants