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

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

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

Patch

Status:Needs review» Closed (duplicate)

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.

  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

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

Disregard this see #7

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

Updated per #5 : , )

Lots of extra whitespace on empty lines.

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

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

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

Whitespace nuked per #8 & #9 : ' )

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

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

Like this?

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

tagging

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.

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

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.

  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.

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

Status:Needs review» Reviewed & tested by the community

Great!

#20 gets a +1 from me as well.

Status:Reviewed & tested by the community» Needs work

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

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.

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.

Status:Needs review» Reviewed & tested by the community

Looks good to me. RTBC!

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

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.

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.

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

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Reviewed & tested by the community

#27 is RTBC.

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?

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.

+++ 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()

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

Status:Needs review» Reviewed & tested by the community

Yeah I am fine with that.

+1 on that.

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.

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.

Status:Needs review» Reviewed & tested by the community

That'll learn ya!

+1 to that!

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!

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

Tiny nitpick of the test function name.

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

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new29.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,613 pass(es).
[ View ]

reroll

Status:Needs review» Reviewed & tested by the community

Back to RTBC

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()"

Issue tags:-Needs reroll

.

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.

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

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

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.

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

Ugh

Back to RTBC.

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.

Status:Needs review» Reviewed & tested by the community

RTBC++. This looks good to me.

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

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new31.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,965 pass(es).
[ View ]

Rerolled.

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Title:Create Base Class for RouteSubscriber ClassChange 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.

Status:Active» Needs review
StatusFileSize
new899 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,552 pass(es).
[ View ]

Here is a small follow up alex requested.

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

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

Now this will even pass.

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.

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

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

Issue summary:View changes

.

Issue summary:View changes
Issue tags:+Needs change record

Issue summary update would be awesome :)

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

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.

Title:Change notice: Create Base Class for RouteSubscriber ClassCreate 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.