The goal here it to define a base class which RouteSubscriber classes will extend providing "getSubscribedEvents" and "abstract public function routes(RouteBuildEvent $event);"

CommentFileSizeAuthor
#68 interdiff.txt702 bytesdawehner
#68 route_subscriber-2098795-68.patch2.6 KBdawehner
#67 route_subscriber-2098795-67.patch2.36 KBdawehner
#67 interdiff.txt595 bytesdawehner
#66 route_subscriber-2098795-66.patch1.78 KBdawehner
#65 route_subscriber-2098795-65.patch899 bytesdawehner
#62 2098795-62.patch31.52 KBdamiankloip
#57 routes-2098795-57.patch31.49 KBtim.plunkett
#57 interdiff.txt752 bytestim.plunkett
#56 routes-2098795-55.patch31.47 KBtim.plunkett
#56 interdiff.txt1.96 KBtim.plunkett
#53 routes-2098795-53.patch31.51 KBtim.plunkett
#53 interdiff.txt9.81 KBtim.plunkett
#49 route-subscriber-base.reroll.patch29.51 KBlarowlan
#47 route_subscriber_base-2098795-47.patch29.5 KBdawehner
#47 interdiff.txt913 bytesdawehner
#46 routing-2098795-46.patch29.21 KBtim.plunkett
#46 interdiff.txt5.62 KBtim.plunkett
#43 routing-2098795-43.patch27.61 KBtim.plunkett
#43 interdiff.txt758 bytestim.plunkett
#41 routing-2098795-41.patch26.87 KBtim.plunkett
#41 interdiff.txt3.04 KBtim.plunkett
#36 routing-2098795-36.patch23.84 KBtim.plunkett
#36 interdiff.txt926 bytestim.plunkett
#31 routing-2098795-31.patch23.66 KBtstoeckler
#31 interdiff2098795-27-31.txt666 byteststoeckler
#27 routing-2098795-27.patch23.58 KBtim.plunkett
#27 interdiff.txt2.69 KBtim.plunkett
#25 interdiff.txt775 bytestim.plunkett
#25 route-2098795-25.patch21.74 KBtim.plunkett
#20 interdiff.txt9.95 KBtim.plunkett
#20 route-2098795-20.patch20.98 KBtim.plunkett
#17 route-2098795-17.patch24.27 KBtim.plunkett
#17 interdiff-all.txt21.84 KBtim.plunkett
#17 interdiff.txt1.52 KBtim.plunkett
#12 routing-2098795-12.patch3.23 KBtim.plunkett
#12 interdiff.txt3.55 KBtim.plunkett
#10 2098795-RouteSubscriberBase-10.patch990 bytesSean Charles
#7 2098795-RouteSubscriberBase-7.patch996 bytesSean Charles
#6 2098795-RouteSubscriberBase-6.patch1001 bytesSean Charles
#2 2098795-RouteSubscriberBase-2.patch898 bytesSean Charles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

This is a duplicate, see the dx initiative at GDO/core

Sean Charles’s picture

Patch

dawehner’s picture

Status: Needs review » Closed (duplicate)
tim.plunkett’s picture

Status: Closed (duplicate) » Needs review

This is an easy win, let's leave #2092529: [meta] Improve DX for defining custom routes for figuring out incremental improvements.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/RouteSubsriberBase.php
    @@ -0,0 +1,30 @@
    + * Definition of Drupal\Core\Routing\RouteSubsriberBase.
    

    Contains \Drupal\

  2. +++ b/core/lib/Drupal/Core/Routing/RouteSubsriberBase.php
    @@ -0,0 +1,30 @@
    +	abstract public function routes(RouteBuildEvent $event);
    

    Indented weird, needs a docblock

Sean Charles’s picture

Disregard this see #7

Sean Charles’s picture

Updated per #5 : , )

brianV’s picture

Lots of extra whitespace on empty lines.

You can see this if you review using DREditor or set your editor to show whitespace characters.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteSubsriberBase.php
@@ -0,0 +1,35 @@
+  ¶
...
+   * Use this to create all dynamic routes.
+   * ¶
...
+	
...
+  ¶

There is some whitespace and tabs everywhere.

Sean Charles’s picture

Whitespace nuked per #8 & #9 : ' )

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteSubsriberBase.php
@@ -0,0 +1,35 @@
+  /**
+   * Use this to create all dynamic routes.
+   *
+   * @param RouteBuildEvent $event
+   */
+  abstract public function routes(RouteBuildEvent $event);

I 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.

tim.plunkett’s picture

FileSize
3.55 KB
3.23 KB

Like this?

tim.plunkett’s picture

It would be trivial to convert the half dozen other route subscribers, but I didn't want to do extra work.

pwolanin’s picture

tagging

dawehner’s picture

What 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.

Crell’s picture

I'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.)

tim.plunkett’s picture

FileSize
1.52 KB
21.84 KB
24.27 KB

Yes, 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()

Status: Needs review » Needs work

The last submitted patch, route-2098795-17.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
    @@ -0,0 +1,73 @@
    +  public function routes(RouteCollection $collection) {
    ...
    +  public function alterRoutes(RouteCollection $collection, $module) {
    

    Should we just make this protected Additional i am wondering whether we should explain here that $module might be also 'dynamic_route'?

  2. +++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
    @@ -0,0 +1,73 @@
    +  public function alterAllRoutes(RouteBuildEvent $event) {
    

    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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
9.95 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

Crell’s picture

#20 gets a +1 from me as well.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, route-2098795-20.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
21.74 KB
775 bytes

Ugh.

Status: Needs review » Needs work

The last submitted patch, route-2098795-25.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
23.58 KB

SpecialAttributesRouteSubscriber is weird and has a return value.

disasm’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. RTBC!

damiankloip’s picture

Yep, +1. I don't really se a better way to go about doing this.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -55,9 +49,9 @@ public function onRouteBuilding(RouteBuildEvent $event) {
   /**
    * {@inheritdoc}
    */
...
+  public function onAlterRoutes(RouteBuildEvent $event) {
+    $collection = $event->getRouteCollection();
+    return $this->alterRoutes($collection, $event->getModule());
   }

I think this deserves a code comment. The additional return value is very easy to overlook.

I'll re-roll this later.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
666 bytes
23.66 KB

Back to RTBC.

damiankloip’s picture

I don't know if we can do inheritdoc and other comments? or at least I don't think we do?

disasm’s picture

Status: Reviewed & tested by the community » Needs work

Either need to completely replace the doc, or just use inheritdoc, can't do both.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

#27 is RTBC.

dawehner’s picture

Nice interdiff nice anyway.

Either need to completely replace the doc, or just use inheritdoc, can't do both.

Well, this adds additional information I think we should keep it?

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
926 bytes
23.84 KB

Then let's actually write docs that explain what is happening.

dawehner’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -47,9 +47,13 @@ protected function alterRoutes(RouteCollection $collection, $module) {
+   * Delegates the route altering to self::alterRoutes().

Well, technically it is actually static::alterRoutes()

tim.plunkett’s picture

We use static:: in code but self:: in docs. Don't ask me why. See also @return self

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah I am fine with that.

damiankloip’s picture

+1 on that.

tim.plunkett’s picture

FileSize
3.04 KB
26.87 KB

Reroll after #2091691: Convert test non-form page callbacks to routes and controllers, converted two recently added route subscribers.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, routing-2098795-41.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
758 bytes
27.61 KB

Serves me right for posting to an RTBC issue.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

That'll learn ya!

damiankloip’s picture

+1 to that!

tim.plunkett’s picture

FileSize
5.62 KB
29.21 KB

Had to reroll the views changes. I ran the unit tests locally this time!

dawehner’s picture

Tiny nitpick of the test function name.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

larowlan’s picture

Status: Needs work » Needs review
FileSize
29.51 KB

reroll

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

andypost’s picture

Assigned: Sean Charles » Unassigned
Status: Reviewed & tested by the community » Needs review

Let's discuss naming a bit more. Suppose getRoutes() and alterRoutes() is better DX

+++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
@@ -0,0 +1,74 @@
+   * Provides new routes by adding them to the collection.
...
+  protected function routes(RouteCollection $collection) {
...
+   * Alters existing routes for a specific collection.
...
+  protected function alterRoutes(RouteCollection $collection, $module) {

The method name makes no sense.
All over core we use consistent pattern for methods "verbTarget()"

Xano’s picture

Issue tags: -Needs reroll

.

tim.plunkett’s picture

FileSize
9.81 KB
31.51 KB

The method name makes no sense.

No, protected function lookAtTheRobotZebras(RouteCollection $collection) { makes no sense. I think you meant that we could slightly improve it.

dawehner’s picture

Well, now we get the routes without actually returning it...

disasm’s picture

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/EventSubscriber/FormTestEventSubscriber.php
@@ -51,18 +51,15 @@ public function onKernelRequest(GetResponseEvent $event) {
   /**
    * Adds routes for the Form Tests.
    */
-  public function routes(RouteBuildEvent $event) {
-    $collection = $event->getRouteCollection();
-    $defaults = array();
-
+  protected function getRoutes(RouteCollection $collection) {

The 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?

tim.plunkett’s picture

FileSize
1.96 KB
31.47 KB

You're 100% right. This is not a getter.
The interdiff is against #49. routes() is fine.

tim.plunkett’s picture

FileSize
752 bytes
31.49 KB

Ugh

dawehner’s picture

Back to RTBC.

tim.plunkett’s picture

There 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.

disasm’s picture

Status: Needs review » Reviewed & tested by the community

RTBC++. This looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
31.52 KB

Rerolled.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

catch’s picture

Title: Create Base Class for RouteSubscriber Class » Change notice: Create Base Class for RouteSubscriber Class
Priority: Normal » Major
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thank!

Could use a change notice.

dawehner’s picture

Status: Active » Needs review
FileSize
899 bytes

Here is a small follow up alex requested.

dawehner’s picture

dawehner’s picture

Now this will even pass.

dawehner’s picture

urgs, it is a clean sign that the special route subscriber should actually fail for real.

Crell’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
@@ -65,10 +65,14 @@ public function onDynamicRoutes(RouteBuildEvent $event) {
+   *
+   * @return bool
+   *   The alterRoutes method can return a boolean value that indicates whether
+   *   it actually altered some routes.
    */
   public function onAlterRoutes(RouteBuildEvent $event) {
     $collection = $event->getRouteCollection();
-    $this->alterRoutes($collection, $event->getModule());
+    return $this->alterRoutes($collection, $event->getModule());

Since event listeners have no return value, what's the purpose of this? The return from a listener is always ignored.

tim.plunkett’s picture

I don't know why we would want this change. The return value is never used in runtime code, only the test for SpecialAttributesRouteSubscriber.

tim.plunkett’s picture

Issue summary: View changes

.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs change record
donquixote’s picture

Issue summary update would be awesome :)

EDIT: Duh, wrong issue. (But issue summary update is always nice)

star-szr’s picture

Change 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.

xjm’s picture

Title: Change notice: Create Base Class for RouteSubscriber Class » Create Base Class for RouteSubscriber Class
Priority: Major » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Agreed, we can move that to a separate followup (although it sounds like is not actually needed?)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.