Fix deprecation notice in route name extraction (#543)

Co-authored-by: Alex Bouma <alex@bouma.me>

Author: X-Coder264

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Fix deprecation notice in route name extraction (#543)
+
 ## 2.11.0
 
 - Add support for Laravel 9 (#534)

src/Sentry/Laravel/EventHandler.php

@@ -279,7 +279,7 @@ public function __call($method, $arguments)
      */
     protected function routerMatchedHandler(Route $route)
     {
-        $routeName = Integration::extractNameForRoute($route) ?? '<unlabeled transaction>';
+        $routeName = Integration::extractNameForRoute($route);
 
         Integration::addBreadcrumb(new Breadcrumb(
             Breadcrumb::LEVEL_INFO,

src/Sentry/Laravel/Integration.php

@@ -30,15 +30,15 @@ class Integration implements IntegrationInterface
      */
     public function setupOnce(): void
     {
-        Scope::addGlobalEventProcessor(function (Event $event): Event {
+        Scope::addGlobalEventProcessor(static function (Event $event): Event {
             $self = SentrySdk::getCurrentHub()->getIntegration(self::class);
 
             if (!$self instanceof self) {
                 return $event;
             }
 
             if (empty($event->getTransaction())) {
-                $event->setTransaction($self->getTransaction());
+                $event->setTransaction(self::getTransaction());
             }
 
             return $event;
@@ -121,52 +121,64 @@ public static function flushEvents(): void
      *
      * @param \Illuminate\Routing\Route $route
      *
-     * @return string|null
+     * @return string
      */
-    public static function extractNameForRoute(Route $route): ?string
+    public static function extractNameForRoute(Route $route): string
     {
         $routeName = null;
 
-        if (empty($routeName) && $route->getName()) {
-            // someaction (route name/alias)
-            $routeName = $route->getName();
-
-            // Laravel 7 route caching generates a route names if the user didn't specify one
-            // theirselfs to optimize route matching. These route names are useless to the
-            // developer so if we encounter a generated route name we discard the value
-            if (Str::contains($routeName, 'generated::')) {
-                $routeName = null;
-            }
-
-            // If the route name ends with a `.` we assume an incomplete group name prefix
-            // we discard this value since it will most likely not mean anything to the
-            // developer and will be duplicated by other unnamed routes in the group
-            if (Str::endsWith($routeName, '.')) {
-                $routeName = null;
-            }
+        // someaction (route name/alias)
+        if ($route->getName()) {
+            $routeName = self::extractNameForNamedRoute($route->getName());
         }
 
+        // Some\Controller@someAction (controller action)
         if (empty($routeName) && $route->getActionName()) {
-            // Some\Controller@someAction (controller action)
-            $routeName = ltrim($route->getActionName(), '\\');
-
-            $baseNamespace = self::$baseControllerNamespace ?? '';
-
-            // Strip away the base namespace from the action name
-            if (!empty($baseNamespace)) {
-                // @see: Str::after, but this is not available before Laravel 5.4 so we use a inlined version
-                $routeName = array_reverse(explode($baseNamespace . '\\', $routeName, 2))[0];
-            }
+            $routeName = self::extractNameForActionRoute($route->getActionName());
         }
 
+        // /someaction // Fallback to the url
         if (empty($routeName) || $routeName === 'Closure') {
-            // /someaction // Fallback to the url
             $routeName = '/' . ltrim($route->uri(), '/');
         }
 
         return $routeName;
     }
 
+    private static function extractNameForNamedRoute(string $routeName): ?string
+    {
+        // Laravel 7 route caching generates a route names if the user didn't specify one
+        // theirselfs to optimize route matching. These route names are useless to the
+        // developer so if we encounter a generated route name we discard the value
+        if (Str::contains($routeName, 'generated::')) {
+            return null;
+        }
+
+        // If the route name ends with a `.` we assume an incomplete group name prefix
+        // we discard this value since it will most likely not mean anything to the
+        // developer and will be duplicated by other unnamed routes in the group
+        if (Str::endsWith($routeName, '.')) {
+            return null;
+        }
+
+        return $routeName;
+    }
+
+    private static function extractNameForActionRoute(string $actionName): string
+    {
+        $routeName = ltrim($actionName, '\\');
+
+        $baseNamespace = self::$baseControllerNamespace ?? '';
+
+        if (empty($baseNamespace)) {
+            return $routeName;
+        }
+
+        // Strip away the base namespace from the action name
+        // @see: Str::after, but this is not available before Laravel 5.4 so we use a inlined version
+        return array_reverse(explode($baseNamespace . '\\', $routeName, 2))[0];
+    }
+
     /**
      * Retrieve the meta tags with tracing information to link this request to front-end requests.
      *

test/Sentry/IntegrationTest.php

@@ -29,9 +29,7 @@ public function testTransactionIsSetWhenRouteMatchedEventIsFired(): void
         Integration::setTransaction(null);
 
         $event = new RouteMatched(
-            new Route('GET', $routeUrl = '/sentry-route-matched-event', static function () {
-                // do nothing...
-            }),
+            new Route('GET', $routeUrl = '/sentry-route-matched-event', []),
             Mockery::mock(Request::class)->makePartial()
         );
 
@@ -45,9 +43,7 @@ public function testTransactionIsSetWhenRouterMatchedEventIsFired(): void
         Integration::setTransaction(null);
 
         $this->dispatchLaravelEvent('router.matched', [
-            new Route('GET', $routeUrl = '/sentry-router-matched-event', static function () {
-                // do nothing...
-            }),
+            new Route('GET', $routeUrl = '/sentry-router-matched-event', []),
         ]);
 
         $this->assertSame($routeUrl, Integration::getTransaction());
@@ -106,4 +102,64 @@ public function testTransactionIsNotAppliedToEventWhenTransactionIsAlreadySet():
             $this->assertSame($eventTransaction, $event->getTransaction());
         });
     }
+
+    public function testExtractingNameForRouteWithName(): void
+    {
+        $route = (new Route('GET', '/foo', []))->name($routeName = 'foo-bar');
+
+        $this->assertSame($routeName, Integration::extractNameForRoute($route));
+    }
+
+    public function testExtractingNameForRouteWithAction(): void
+    {
+        $route = (new Route('GET', '/foo', [
+            'controller' => $controller = 'SomeController@someAction',
+        ]));
+
+        $this->assertSame($controller, Integration::extractNameForRoute($route));
+    }
+
+    public function testExtractingNameForRouteWithoutName(): void
+    {
+        $route = new Route('GET', $url = '/foo', []);
+
+        $this->assertSame($url, Integration::extractNameForRoute($route));
+    }
+
+    public function testExtractingNameForRouteWithActionAndName(): void
+    {
+        $route = (new Route('GET', '/foo', [
+            'controller' => 'SomeController@someAction',
+        ]))->name($routeName = 'foo-bar');
+
+        $this->assertSame($routeName, Integration::extractNameForRoute($route));
+    }
+
+    public function testExtractingNameForRouteWithAutoGeneratedName(): void
+    {
+        // We fake a generated name here, Laravel generates them each starting with `generated::`
+        $route = (new Route('GET', $url = '/foo', []))->name('generated::KoAePbpBofo01ey4');
+
+        $this->assertSame($url, Integration::extractNameForRoute($route));
+    }
+
+    public function testExtractingNameForRouteWithIncompleteGroupName(): void
+    {
+        $route = (new Route('GET', $url = '/foo', []))->name('group-name.');
+
+        $this->assertSame($url, Integration::extractNameForRoute($route));
+    }
+
+    public function testExtractingNameForRouteWithStrippedBaseNamespaceFromAction(): void
+    {
+        Integration::setControllersBaseNamespace('BaseNamespace');
+
+        $route = (new Route('GET', '/foo', [
+            'controller' => 'BaseNamespace\\SomeController@someAction',
+        ]));
+
+        $this->assertSame('SomeController@someAction', Integration::extractNameForRoute($route));
+
+        Integration::setControllersBaseNamespace(null);
+    }
 }