Skip to content
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

[5.0] Route Cache Improvements #7432

Closed
wants to merge 1 commit into from
Closed

[5.0] Route Cache Improvements #7432

wants to merge 1 commit into from

Conversation

baileylo
Copy link
Contributor

My Issue

I had two routes with the same URI and method but different names. When I tried to cache the routes the command would fail with a serialization error.

Solution

I dug into the code and figured out that when routes were prepared for serialization only one of the arrays of routes was iterated over. There were other routes in the collection in other arrays that would not be prepare for serialization. Following tell don't ask, rather than asking for all the routes in the collection and prepare the routes for serialization, I added a method to tell the collection to prepare it for serialization.

Side Notes

I feel like this seems rather awkward so I feel I should explain why I have two routes with the same URi and method but with different names. My site is a blog and has both permalink pages as well as blog posts. Both can be access from .tld/{slug}. If there was no post that could match {slug} then the app would check for a matching page. I wanted to keep the route generation separate for the 2 objects in case I decided to change the uris later. This was the original issue, I found it also applied to action as well as header differences and included a solution that solved all three issues.

*/
public function prepareForSerialization()
{
foreach($this->routes as $routes)
Copy link
Member

Choose a reason for hiding this comment

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

We need a space before the opening bracket.

@baileylo
Copy link
Contributor Author

@GrahamCampbell I believe I fixed all of the coding style issues.

// Serialize the Routes
serialize($this->collection);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line before the closing brace.

@baileylo
Copy link
Contributor Author

@GrahamCampbell Added your suggested changes. I also moved the new public method so it wasn't between the protected methods called from RouteCollection::add.

@GrahamCampbell
Copy link
Member

Ok, now we wait for Taylor then. :)

@baileylo
Copy link
Contributor Author

@GrahamCampbell Should the "requires changes" tag be removed?

@jobrios
Copy link

jobrios commented Feb 16, 2015

But, for the case of same URI and method but different names (different actions), is it acceptable for both members of a route-instance pair to exist in $nameList ($actionList), while only one of them exists in $routes and $allRoutes due to overwriting? The overwriting occurs because these latter two arrays are keyed by URI and method. See Issue #7452.

@GrahamCampbell
Copy link
Member

Should the "requires changes" tag be removed?

No, I'm waiting for you to squash to one commit.

@GrahamCampbell GrahamCampbell changed the title Route Cache Improvements [5.0] Route Cache Improvements Feb 16, 2015
Fixes issues in caching route commands related to routes with duplicate
paths and methods but differing actions or names as well as routes with
duplicates paths but differing methods.
@baileylo
Copy link
Contributor Author

@GrahamCampbell commits squashed

@taylorotwell
Copy link
Member

Kind of edge-casey.

@sebestenyb
Copy link

sebestenyb commented May 4, 2016

I ran into this problem with Spark.

  1. Spark uses the /terms to display the content of the terms.md file within the default layout, we'd prefer have another layout for that file, can't overwrite the route to use different controller/view.
  2. Cashier webhooks, we can't use /webhook/braintree or /webhook/stripe with an overwritten controller

Both cases assumes that you can actually overwrite a route to use different controller, but then you can't use route caching.

@ellisio
Copy link
Contributor

ellisio commented Jul 8, 2016

Running into this problem with Spark as well. We are overriding some Spark routes for internal use and it causes route:cache to throw Serialization of 'Closure' is not allowed.

@manik005
Copy link

@ellisio did you managed to sort it out ?

@ellisio
Copy link
Contributor

ellisio commented Jan 12, 2017

@manik005 Unfortunately this issue was from almost 6 months ago and I don't really remember what was going on. We ended up going a different route with the product this was in :(

@manik005
Copy link

@ellisio oh! alright, thanks for the response :)

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

Successfully merging this pull request may close these issues.

7 participants