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.5] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). #20820

Closed
wants to merge 8 commits into
base: 5.5
from

Conversation

Projects
None yet
7 participants
@DanielCoulbourne
Contributor

DanielCoulbourne commented Aug 29, 2017

I've always wanted to do more with Route groups, but because they have been primarily used as stacks of attributes which were immediately flattened and didn't persist outside of their registration, there wasn't much to do.

This PR introduces a RouteGroup object and a $groups array on Router which is a persistent registry of RouteGroups. RouteGroup objects are aware of their routes via a RouteCollection.

I also added a getGroups() method to Router.

While pretty in depth in its changes, this is just an introductory "testing the waters" PR. If this PR is merged there are lots of things that could be built on top of persistent route groups:

  • Routes could know their groups, allowing $router->getRoutes()->getRoutesByGroup()
  • Groups could be refactored to contain all of their own attributes and know their parent groups, allowing all of the "group stack" and "attribute flattening" logic to live inside RouteGroup (which seems more natural to me)

Note that the classname RouteGroup was already in use for a class of static methods relating to merging Route Group Attributes. I renamed this class to RouteGroupAttributes

Please let me know what you think of this PR. I think its a step in the right direction. I love Route Groups and would love to do more work with them, but I think they deserve a promotion.

@browner12

This comment has been minimized.

Show comment
Hide comment
@browner12

browner12 Aug 29, 2017

Contributor

I'm not super familiar with the code here, so I can't speak to that, but the idea sounds really interesting.

Contributor

browner12 commented Aug 29, 2017

I'm not super familiar with the code here, so I can't speak to that, but the idea sounds really interesting.

@tillkruss tillkruss changed the title from Promote RouteGroup to first-class citizen, enabling $router->getGroups(). to [5.5] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). Aug 30, 2017

@tillkruss

This comment has been minimized.

Show comment
Hide comment
@tillkruss

tillkruss Aug 30, 2017

Member

That will have to go to v5.6 now.

Member

tillkruss commented Aug 30, 2017

That will have to go to v5.6 now.

@GrahamCampbell GrahamCampbell changed the title from [5.5] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). to [5.6] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). Aug 30, 2017

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Aug 30, 2017

Contributor

Just noting that I don't think this actually needs to go in 5.6
There are no breaking changes and all the 5.5 tests still pass. I think this is minor enough that it can go in 5.5

Contributor

DanielCoulbourne commented Aug 30, 2017

Just noting that I don't think this actually needs to go in 5.6
There are no breaking changes and all the 5.5 tests still pass. I think this is minor enough that it can go in 5.5

@DanielCoulbourne DanielCoulbourne changed the title from [5.6] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). to [5.5] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). Sep 5, 2017

@GrahamCampbell GrahamCampbell changed the title from [5.5] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). to [5.6] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). Sep 5, 2017

@tillkruss tillkruss changed the base branch from master to 5.5 Sep 6, 2017

@tillkruss tillkruss changed the title from [5.6] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). to [5.5] Promote RouteGroup to first-class citizen, enabling $router->getGroups(). Sep 6, 2017

@tillkruss

This comment has been minimized.

Show comment
Hide comment
@tillkruss

tillkruss Sep 6, 2017

Member

I changed the branch to v5.5.

Member

tillkruss commented Sep 6, 2017

I changed the branch to v5.5.

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Sep 6, 2017

Contributor

Oh yeah thanks! Shoulda remembered that.

Contributor

DanielCoulbourne commented Sep 6, 2017

Oh yeah thanks! Shoulda remembered that.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 6, 2017

Member

What will this enable application developers to do?

Member

taylorotwell commented Sep 6, 2017

What will this enable application developers to do?

@hawkleaf

This comment has been minimized.

Show comment
Hide comment
@hawkleaf

hawkleaf Sep 8, 2017

I believe this was tied to this package:
https://github.com/tightenco/ziggy

I'm not sure what exactly it was for, it can be found on the twenty percent time podcast. Would be nice if @DanielCoulbourne could explain though ^^

hawkleaf commented Sep 8, 2017

I believe this was tied to this package:
https://github.com/tightenco/ziggy

I'm not sure what exactly it was for, it can be found on the twenty percent time podcast. Would be nice if @DanielCoulbourne could explain though ^^

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Sep 9, 2017

Contributor

@taylorotwell This pull itself will only enable devs to get the route groups. Probably not the most useful feature on its own. But like I said above this is kind of a "test the waters" PR to see if you're even open to this sort of thing. If you are, there are a few ancillary things I'd like to PR in that I do have a need for:

The biggest one is building $routeCollection->getRoutesByGroup() which would be SUPER useful to me in building out configuration options for Ziggy (but also could be useful to anyone who wants to do stuff with group-structured routes). This could even allow me to write a Route::macro that would let me do something like Route::getResources() (since resources appear to just be groups with a wrapper function) which would be awesome if someone (who may or may not be me) was dreaming of building a frontend client for resources.

Contributor

DanielCoulbourne commented Sep 9, 2017

@taylorotwell This pull itself will only enable devs to get the route groups. Probably not the most useful feature on its own. But like I said above this is kind of a "test the waters" PR to see if you're even open to this sort of thing. If you are, there are a few ancillary things I'd like to PR in that I do have a need for:

The biggest one is building $routeCollection->getRoutesByGroup() which would be SUPER useful to me in building out configuration options for Ziggy (but also could be useful to anyone who wants to do stuff with group-structured routes). This could even allow me to write a Route::macro that would let me do something like Route::getResources() (since resources appear to just be groups with a wrapper function) which would be awesome if someone (who may or may not be me) was dreaming of building a frontend client for resources.

@m1guelpf

This comment has been minimized.

Show comment
Hide comment
@m1guelpf

m1guelpf Sep 9, 2017

Contributor

For reference, this PR, its implications, etc. were discussed in Twenty Percent Time.

Contributor

m1guelpf commented Sep 9, 2017

For reference, this PR, its implications, etc. were discussed in Twenty Percent Time.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 12, 2017

Member

Could you take a look at the conflicts on this? I plan to pull it down today to play with it.

Member

taylorotwell commented Sep 12, 2017

Could you take a look at the conflicts on this? I plan to pull it down today to play with it.

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Sep 12, 2017

Contributor

Yep just a sec

(I broke something)

Contributor

DanielCoulbourne commented Sep 12, 2017

Yep just a sec

(I broke something)

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Sep 12, 2017

Contributor

@taylorotwell fixed. Sorry about that. I was trying to resolve merge conflicts in the branch I used for the 5.4 version and lost my mind for a bit. Derp.

Contributor

DanielCoulbourne commented Sep 12, 2017

@taylorotwell fixed. Sorry about that. I was trying to resolve merge conflicts in the branch I used for the 5.4 version and lost my mind for a bit. Derp.

@m1guelpf

This comment has been minimized.

Show comment
Hide comment
@m1guelpf

m1guelpf Sep 22, 2017

Contributor

@taylorotwell Any updates?

Contributor

m1guelpf commented Sep 22, 2017

@taylorotwell Any updates?

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Sep 23, 2017

Contributor

@m1guelpf We've been talking back and forth on it a bit. I know Taylor has taken a look at it.

Contributor

DanielCoulbourne commented Sep 23, 2017

@m1guelpf We've been talking back and forth on it a bit. I know Taylor has taken a look at it.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Sep 23, 2017

Member

There definitely are breaking changes. Protected methods have been deleted from non-final classes. Should go to 5.6.

Member

GrahamCampbell commented Sep 23, 2017

There definitely are breaking changes. Protected methods have been deleted from non-final classes. Should go to 5.6.

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Sep 25, 2017

Member

Going to hold off for now until I see the end-goal macro implemented using this PR's functionality. Without that I'm scared we don't have hard proof this acutally solves what we're trying to achieve.

Member

taylorotwell commented Sep 25, 2017

Going to hold off for now until I see the end-goal macro implemented using this PR's functionality. Without that I'm scared we don't have hard proof this acutally solves what we're trying to achieve.

@DanielCoulbourne

This comment has been minimized.

Show comment
Hide comment
@DanielCoulbourne

DanielCoulbourne Sep 26, 2017

Contributor

Challenge accepted 😛
It's gonna be a week or two though.

Contributor

DanielCoulbourne commented Sep 26, 2017

Challenge accepted 😛
It's gonna be a week or two though.

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