Merge pull request #16431 from phalcon/router-memory-leak

Fix memory leak in `Router::handle()`
phalcon · Sep 11, 2023 · 620e7a9 · 620e7a9
2 parents 42c5a39 + eccd93f
commit 620e7a9
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 33 deletions.
diff --git a/CHANGELOG-5.0.md b/CHANGELOG-5.0.md
@@ -3,8 +3,9 @@
 ## [5.3.1](https://github.com/phalcon/cphalcon/releases/tag/v5.3.1) (xxxx-xx-xx)
 
 ### Fixed
-- Infinite save loop in Model::save() [#16395](https://github.com/phalcon/cphalcon/issues/16395)
+- Infinite save loop in `Phalcon\Mvc\Model::save()` [#16395](https://github.com/phalcon/cphalcon/issues/16395)
 - Undefined column with columnMap and model caching [#16420] (https://github.com/phalcon/cphalcon/issues/16420)
+- Fixed memory leak in `Phalcon\Mvc\Router::handle()` [#16431] (https://github.com/phalcon/cphalcon/pull/16431)
 
 
 ## [5.3.0](https://github.com/phalcon/cphalcon/releases/tag/v5.3.0) (2023-08-15)

diff --git a/phalcon/Mvc/Router.zep b/phalcon/Mvc/Router.zep
@@ -701,8 +701,7 @@ class Router extends AbstractInjectionAware implements RouterInterface, EventsAw
             let handledUri = "/";
         }
 
-        let request = null,
-            currentHostName = null,
+        let currentHostName = null,
             routeFound = false,
             parts = [],
             params = [],
@@ -711,11 +710,22 @@ class Router extends AbstractInjectionAware implements RouterInterface, EventsAw
             this->matchedRoute = null;
 
         let eventsManager = this->eventsManager;
-
         if eventsManager !== null {
             eventsManager->fire("router:beforeCheckRoutes", this);
         }
 
+        /**
+         * Retrieve the request service from the container
+         */
+        let container = <DiInterface> this->container;
+        if container === null {
+            throw new Exception(
+                "A dependency injection container is required to access the 'request' service"
+            );
+        }
+
+        let request = <RequestInterface> container->get("request");
+
         /**
          * Routes are traversed in reversed order
          */
@@ -728,20 +738,6 @@ class Router extends AbstractInjectionAware implements RouterInterface, EventsAw
              */
             let methods = route->getHttpMethods();
             if methods !== null {
-                /**
-                 * Retrieve the request service from the container
-                 */
-                if request === null {
-                    let container = <DiInterface> this->container;
-                    if container === null {
-                        throw new Exception(
-                            "A dependency injection container is required to access the 'request' service"
-                        );
-                    }
-
-                    let request = <RequestInterface> container->getShared("request");
-                }
-
                 /**
                  * Check if the current method is allowed by the route
                  */
@@ -755,20 +751,6 @@ class Router extends AbstractInjectionAware implements RouterInterface, EventsAw
              */
             let hostname = route->getHostName();
             if hostname !== null {
-                /**
-                 * Retrieve the request service from the container
-                 */
-                if request === null {
-                    let container = <DiInterface> this->container;
-                    if container === null {
-                        throw new Exception(
-                            "A dependency injection container is required to access the 'request' service"
-                        );
-                    }
-
-                    let request = <RequestInterface> container->getShared("request");
-                }
-
                 /**
                  * Check if the current hostname is the same as the route
                  */

diff --git a/tests/integration/Mvc/Router/HandleCest.php b/tests/integration/Mvc/Router/HandleCest.php
@@ -34,6 +34,7 @@ public function mvcRouterHandle(IntegrationTester $I)
         $I->wantToTest('Mvc\Router - handle()');
 
         $router = new Router();
+        $router->setDI(new FactoryDefault());
 
         $router->add(
             '/admin/invoices/list',
@@ -77,6 +78,7 @@ public function mvcRouterHandleWithPlaceholders(IntegrationTester $I)
          * Regular placeholders
          */
         $router = new Router(false);
+        $router->setDI(new FactoryDefault());
         $router->add(
             '/:module/:namespace/:controller/:action/:params/:int',
             [
@@ -227,6 +229,7 @@ public function mvcRouterHandleShortSyntax(IntegrationTester $I)
         $I->wantToTest('Mvc\Router - handle() - short syntax');
 
         $router = new Router(false);
+        $router->setDI(new FactoryDefault());
         $router->add("/about", "About::content");
 
         $router->handle('/about');

diff --git a/tests/integration/Mvc/Router/Refactor/GroupCest.php b/tests/integration/Mvc/Router/Refactor/GroupCest.php
@@ -15,6 +15,7 @@
 
 use Codeception\Example;
 use IntegrationTester;
+use Phalcon\Di\FactoryDefault;
 use Phalcon\Mvc\Router;
 use Phalcon\Mvc\Router\Group;
 use Phalcon\Mvc\Router\Route;
@@ -38,6 +39,7 @@ public function testGroups(IntegrationTester $I, Example $example)
         Route::reset();
 
         $router = new Router(false);
+        $router->setDI(new FactoryDefault());
 
         $blog = new Group(
             [
@@ -143,7 +145,6 @@ public function testHostnameRouteGroup(IntegrationTester $I, Example $example)
         $container = $this->getDi();
 
         $router = new Router(false);
-
         $router->setDI($container);
 
         $router->add(