Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The goal here it to define a base class which RouteSubscriber classes will extend providing "getSubscribedEvents" and "abstract public function routes(RouteBuildEvent $event);"
Comment | File | Size | Author |
---|---|---|---|
#68 | interdiff.txt | 702 bytes | dawehner |
#68 | route_subscriber-2098795-68.patch | 2.6 KB | dawehner |
#67 | route_subscriber-2098795-67.patch | 2.36 KB | dawehner |
#67 | interdiff.txt | 595 bytes | dawehner |
#66 | route_subscriber-2098795-66.patch | 1.78 KB | dawehner |
Comments
Comment #1
larowlanThis is a duplicate, see the dx initiative at GDO/core
Comment #2
Sean Charles CreditAttribution: Sean Charles commentedPatch
Comment #3
dawehner#2092529: [meta] Improve DX for defining custom routes is the duplicate.
Comment #4
tim.plunkettThis is an easy win, let's leave #2092529: [meta] Improve DX for defining custom routes for figuring out incremental improvements.
Comment #5
tim.plunkettContains \Drupal\
Indented weird, needs a docblock
Comment #6
Sean Charles CreditAttribution: Sean Charles commentedDisregard this see #7
Comment #7
Sean Charles CreditAttribution: Sean Charles commentedUpdated per #5 : , )
Comment #8
brianV CreditAttribution: brianV commentedLots of extra whitespace on empty lines.
You can see this if you review using DREditor or set your editor to show whitespace characters.
Comment #9
dawehnerThere is some whitespace and tabs everywhere.
Comment #10
Sean Charles CreditAttribution: Sean Charles commentedWhitespace nuked per #8 & #9 : ' )
Comment #11
dawehnerI am wondering whether we could add a new helper method which gets the route collection and the module name instead of the route build event.
Comment #12
tim.plunkettLike this?
Comment #13
tim.plunkettIt would be trivial to convert the half dozen other route subscribers, but I didn't want to do extra work.
Comment #14
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #15
dawehnerWhat do you think about adding a method for altering as well? It seems that altering a route entry is also a really common usecase given my experience with hook_menu in Drupal 7.
Comment #16
Crell CreditAttribution: Crell commentedI'm OK with this, but weren't we already discussing finding a different mechanism than events for dynamic routes anyway in Prague? (Not sure we should delay on that, just noting it.)
Comment #17
tim.plunkettYes, but every one of those discussions quickly devolves into "routes as plugins, use derivatives", and we should be able to make some quick wins now.
This changes to provide an alter method, and knocks out the remaining conversions (no follow-ups needed webchick!)
Will need a trivial reroll after #2091411: Provide an easier mechanism for a route definition wrapped by module_exists()
Comment #19
dawehnerShould we just make this protected Additional i am wondering whether we should explain here that $module might be also 'dynamic_route'?
Sorry but this function name describes the wrong thing. The alter event is fired once for every module .routing.yml file as well as once for the dynamic event. This all confused me.
Comment #20
tim.plunkettGood points!
Patch is smaller because #2091411: Provide an easier mechanism for a route definition wrapped by module_exists() went in
Comment #21
dawehnerGreat!
Comment #22
Crell CreditAttribution: Crell commented#20 gets a +1 from me as well.
Comment #23
tim.plunkettSee #2102417: Change Drupal\Core\Routing\RouteBuildEvent::getModule() to getProvider() for a follow-up
Comment #25
tim.plunkettUgh.
Comment #27
tim.plunkettSpecialAttributesRouteSubscriber is weird and has a return value.
Comment #28
disasm CreditAttribution: disasm commentedLooks good to me. RTBC!
Comment #29
damiankloip CreditAttribution: damiankloip commentedYep, +1. I don't really se a better way to go about doing this.
Comment #30
tstoecklerI think this deserves a code comment. The additional return value is very easy to overlook.
I'll re-roll this later.
Comment #31
tstoecklerBack to RTBC.
Comment #32
damiankloip CreditAttribution: damiankloip commentedI don't know if we can do inheritdoc and other comments? or at least I don't think we do?
Comment #33
disasm CreditAttribution: disasm commentedEither need to completely replace the doc, or just use inheritdoc, can't do both.
Comment #34
tim.plunkett#27 is RTBC.
Comment #35
dawehnerNice interdiff nice anyway.
Well, this adds additional information I think we should keep it?
Comment #36
tim.plunkettThen let's actually write docs that explain what is happening.
Comment #37
dawehnerWell, technically it is actually static::alterRoutes()
Comment #38
tim.plunkettWe use static:: in code but self:: in docs. Don't ask me why. See also
@return self
Comment #39
dawehnerYeah I am fine with that.
Comment #40
damiankloip CreditAttribution: damiankloip commented+1 on that.
Comment #41
tim.plunkettReroll after #2091691: Convert test non-form page callbacks to routes and controllers, converted two recently added route subscribers.
Comment #43
tim.plunkettServes me right for posting to an RTBC issue.
Comment #44
Crell CreditAttribution: Crell commentedThat'll learn ya!
Comment #45
damiankloip CreditAttribution: damiankloip commented+1 to that!
Comment #46
tim.plunkettHad to reroll the views changes. I ran the unit tests locally this time!
Comment #47
dawehnerTiny nitpick of the test function name.
Comment #48
alexpottPatch no longer applies.
Comment #49
larowlanreroll
Comment #50
dawehnerBack to RTBC
Comment #51
andypostLet's discuss naming a bit more. Suppose
getRoutes()
andalterRoutes()
is better DXThe method name makes no sense.
All over core we use consistent pattern for methods "verbTarget()"
Comment #52
Xano.
Comment #53
tim.plunkettNo,
protected function lookAtTheRobotZebras(RouteCollection $collection) {
makes no sense. I think you meant that we could slightly improve it.Comment #54
dawehnerWell, now we get the routes without actually returning it...
Comment #55
disasm CreditAttribution: disasm commentedThe doctag and the function name don't agree. The doc tag says this is adding routes, whereas the function is called getRoutes. Which one is it doing?
Comment #56
tim.plunkettYou're 100% right. This is not a getter.
The interdiff is against #49. routes() is fine.
Comment #57
tim.plunkettUgh
Comment #58
dawehnerBack to RTBC.
Comment #59
tim.plunkettThere are 10 conversions here. 6 of them were named routes() before, the others were named several different things. This standardizes it. If you'd like to propose other names (not getRoutes or addRoutes), do so in another issue.
Comment #60
disasm CreditAttribution: disasm commentedRTBC++. This looks good to me.
Comment #61
alexpottPatch no longer applies.
Comment #62
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #63
jibranBack to RTBC.
Comment #64
catchCommitted/pushed to 8.x, thank!
Could use a change notice.
Comment #65
dawehnerHere is a small follow up alex requested.
Comment #66
dawehnerChange-notice is on http://drupal.org/node/2114953
Comment #67
dawehnerNow this will even pass.
Comment #68
dawehnerurgs, it is a clean sign that the special route subscriber should actually fail for real.
Comment #69
Crell CreditAttribution: Crell commentedSince event listeners have no return value, what's the purpose of this? The return from a listener is always ignored.
Comment #70
tim.plunkettI don't know why we would want this change. The return value is never used in runtime code, only the test for SpecialAttributesRouteSubscriber.
Comment #70.0
tim.plunkett.
Comment #71
xjmComment #72
donquixote CreditAttribution: donquixote commentedIssue summary update would be awesome :)
EDIT: Duh, wrong issue. (But issue summary update is always nice)
Comment #73
star-szrChange record (http://drupal.org/node/2114953) created by @dawehner (see #66). I'd like to suggest that the (presumably not major) follow-up that was requested be moved to a new issue for further discussion so we can close out this issue.
Comment #74
xjmAgreed, we can move that to a separate followup (although it sounds like is not actually needed?)