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

Files: 
CommentFileSizeAuthor
#68 interdiff.txt702 bytesdawehner
#68 route_subscriber-2098795-68.patch2.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es).
[ View ]
#67 route_subscriber-2098795-67.patch2.36 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]
#67 interdiff.txt595 bytesdawehner
#66 route_subscriber-2098795-66.patch1.78 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,051 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
#65 route_subscriber-2098795-65.patch899 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,552 pass(es).
[ View ]
#62 2098795-62.patch31.52 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,965 pass(es).
[ View ]
#57 routes-2098795-57.patch31.49 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,749 pass(es).
[ View ]
#57 interdiff.txt752 bytestim.plunkett
#56 routes-2098795-55.patch31.47 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#56 interdiff.txt1.96 KBtim.plunkett
#53 routes-2098795-53.patch31.51 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,226 pass(es).
[ View ]
#53 interdiff.txt9.81 KBtim.plunkett
#49 route-subscriber-base.reroll.patch29.51 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,613 pass(es).
[ View ]
#47 route_subscriber_base-2098795-47.patch29.5 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,457 pass(es).
[ View ]
#47 interdiff.txt913 bytesdawehner
#46 routing-2098795-46.patch29.21 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]
#46 interdiff.txt5.62 KBtim.plunkett
#43 routing-2098795-43.patch27.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,699 pass(es).
[ View ]
#43 interdiff.txt758 bytestim.plunkett
#41 routing-2098795-41.patch26.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#41 interdiff.txt3.04 KBtim.plunkett
#36 routing-2098795-36.patch23.84 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,901 pass(es).
[ View ]
#36 interdiff.txt926 bytestim.plunkett
#31 routing-2098795-31.patch23.66 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,247 pass(es).
[ View ]
#31 interdiff2098795-27-31.txt666 byteststoeckler
#27 routing-2098795-27.patch23.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]
#27 interdiff.txt2.69 KBtim.plunkett
#25 interdiff.txt775 bytestim.plunkett
#25 route-2098795-25.patch21.74 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#20 interdiff.txt9.95 KBtim.plunkett
#20 route-2098795-20.patch20.98 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#17 route-2098795-17.patch24.27 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#17 interdiff-all.txt21.84 KBtim.plunkett
#17 interdiff.txt1.52 KBtim.plunkett
#12 routing-2098795-12.patch3.23 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,179 pass(es).
[ View ]
#12 interdiff.txt3.55 KBtim.plunkett
#10 2098795-RouteSubscriberBase-10.patch990 bytesSean Charles
PASSED: [[SimpleTest]]: [MySQL] 58,653 pass(es).
[ View ]
#7 2098795-RouteSubscriberBase-7.patch996 bytesSean Charles
PASSED: [[SimpleTest]]: [MySQL] 58,637 pass(es).
[ View ]
#6 2098795-RouteSubscriberBase-6.patch1001 bytesSean Charles
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]
#2 2098795-RouteSubscriberBase-2.patch898 bytesSean Charles
PASSED: [[SimpleTest]]: [MySQL] 58,670 pass(es).
[ View ]

Comments

larowlan’s picture

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

Sean Charles’s picture

StatusFileSize
new898 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,670 pass(es).
[ View ]

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

StatusFileSize
new1001 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]

Disregard this see #7

Sean Charles’s picture

StatusFileSize
new996 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,637 pass(es).
[ View ]

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

StatusFileSize
new990 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,653 pass(es).
[ View ]

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

StatusFileSize
new3.55 KB
new3.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,179 pass(es).
[ View ]

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

StatusFileSize
new1.52 KB
new21.84 KB
new24.27 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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
StatusFileSize
new20.98 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new9.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
StatusFileSize
new21.74 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new775 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
StatusFileSize
new2.69 KB
new23.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]

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
StatusFileSize
new666 bytes
new23.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,247 pass(es).
[ View ]

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
StatusFileSize
new926 bytes
new23.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,901 pass(es).
[ View ]

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

StatusFileSize
new3.04 KB
new26.87 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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
StatusFileSize
new758 bytes
new27.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,699 pass(es).
[ View ]

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

StatusFileSize
new5.62 KB
new29.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,449 pass(es).
[ View ]

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

dawehner’s picture

StatusFileSize
new913 bytes
new29.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,457 pass(es).
[ View ]

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
StatusFileSize
new29.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,613 pass(es).
[ View ]

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

StatusFileSize
new9.81 KB
new31.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,226 pass(es).
[ View ]

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

StatusFileSize
new1.96 KB
new31.47 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new752 bytes
new31.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,749 pass(es).
[ View ]

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
StatusFileSize
new31.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,965 pass(es).
[ View ]

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
StatusFileSize
new899 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,552 pass(es).
[ View ]

Here is a small follow up alex requested.

dawehner’s picture

StatusFileSize
new1.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,051 pass(es), 14 fail(s), and 0 exception(s).
[ View ]

Change-notice is on http://drupal.org/node/2114953

dawehner’s picture

StatusFileSize
new595 bytes
new2.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]

Now this will even pass.

dawehner’s picture

StatusFileSize
new2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es).
[ View ]
new702 bytes

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)

Cottser’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.