New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Router.php namedRoutes caching #1738

Merged
merged 2 commits into from Feb 6, 2016

Conversation

Projects
None yet
3 participants
@jacklynmatts
Contributor

jacklynmatts commented Jan 14, 2016

pathFor() should function properly ( and not throw an exception) during the following sequence.

  1. call pathFor()
  2. map() a new named route
  3. call pathFor() for the route added in step 2.

Therefore, getNamedRoute() should be more precise than is_null($this->namedRoutes). Comparing the size of namedRoutes to routes, while not perfect, is better.

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Feb 5, 2016

Member

Shouldn't we be invalidating namedRoutes when map is called?

Member

akrabat commented Feb 5, 2016

Shouldn't we be invalidating namedRoutes when map is called?

@akrabat akrabat added the bug label Feb 5, 2016

@akrabat akrabat added this to the 3.2.0 milestone Feb 5, 2016

@jacklynmatts

This comment has been minimized.

Show comment
Hide comment
@jacklynmatts

jacklynmatts Feb 5, 2016

Contributor

That would be a better solution. Unfortunately, after thinking about this a little further, I'm not sure even that fully solves the problem.

It's absolutely possible to call Route::setName multiple times. This means that the route name could change even after map is called - and I can't think of any way to observe that inside of Router. This is, to be a fair, an edge case.

I think the simplest way to solve that would be to completely eliminate namedRoutes, unless you can think of something else?

Contributor

jacklynmatts commented Feb 5, 2016

That would be a better solution. Unfortunately, after thinking about this a little further, I'm not sure even that fully solves the problem.

It's absolutely possible to call Route::setName multiple times. This means that the route name could change even after map is called - and I can't think of any way to observe that inside of Router. This is, to be a fair, an edge case.

I think the simplest way to solve that would be to completely eliminate namedRoutes, unless you can think of something else?

@sharifzadesina

This comment has been minimized.

Show comment
Hide comment
@sharifzadesina

sharifzadesina Feb 5, 2016

hi
The only way for fixing this problem is removing Route::setName method and insert a feature for registering named routes like other routes in App class.
for more info see elegantweb/framework routing in github :D
Route class and RouteCollection class

sharifzadesina commented Feb 5, 2016

hi
The only way for fixing this problem is removing Route::setName method and insert a feature for registering named routes like other routes in App class.
for more info see elegantweb/framework routing in github :D
Route class and RouteCollection class

remove namedRoutes cache
Updated based on feedback / conversation on pull request
@jacklynmatts

This comment has been minimized.

Show comment
Hide comment
@jacklynmatts

jacklynmatts Feb 5, 2016

Contributor

Thanks @n0o0bSina , perhaps I should have been more clear. I wasn't suggesting that the named routes feature be removed. Rather, I'm now suggesting that getNamedRoute loops through each Route object in routes on every call, as this seems the only way to ensure we're working with accurate, current information. This would result in removal of the Router::namedRoutes property, since it would be unused.

Sure, it's less performant, but that seems like a secondary concern to being correct. Does it make sense to just correct the error, and address any performance issues in another PR?

I've updated the pull request taking this conversation into consideration.

Contributor

jacklynmatts commented Feb 5, 2016

Thanks @n0o0bSina , perhaps I should have been more clear. I wasn't suggesting that the named routes feature be removed. Rather, I'm now suggesting that getNamedRoute loops through each Route object in routes on every call, as this seems the only way to ensure we're working with accurate, current information. This would result in removal of the Router::namedRoutes property, since it would be unused.

Sure, it's less performant, but that seems like a secondary concern to being correct. Does it make sense to just correct the error, and address any performance issues in another PR?

I've updated the pull request taking this conversation into consideration.

@sharifzadesina

This comment has been minimized.

Show comment
Hide comment
@sharifzadesina

sharifzadesina Feb 5, 2016

@jacklynmatts there is more better way if you don't like standards :D You can inject Router class into each Route then set key in both Route and Router.

sharifzadesina commented Feb 5, 2016

@jacklynmatts there is more better way if you don't like standards :D You can inject Router class into each Route then set key in both Route and Router.

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Feb 6, 2016

Member

Ah - so the problem is that the Router holds the list of named routes, but we set the name on the Route, so we can't invalidate the cached list in Router.

Member

akrabat commented Feb 6, 2016

Ah - so the problem is that the Router holds the list of named routes, but we set the name on the Route, so we can't invalidate the cached list in Router.

@akrabat akrabat merged commit a59ed4c into slimphp:3.x Feb 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

akrabat added a commit that referenced this pull request Feb 6, 2016

@akrabat

This comment has been minimized.

Show comment
Hide comment
@akrabat

akrabat Feb 6, 2016

Member

Thanks for finding and fixing this.

Member

akrabat commented Feb 6, 2016

Thanks for finding and fixing this.

@akrabat akrabat added bug fix and removed bug labels Feb 6, 2016

@jacklynmatts

This comment has been minimized.

Show comment
Hide comment
@jacklynmatts

jacklynmatts Feb 6, 2016

Contributor

Thanks!

Contributor

jacklynmatts commented Feb 6, 2016

Thanks!

@jacklynmatts jacklynmatts deleted the jacklynmatts:getNamedRoute branch Feb 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment