Fixed rare double execution of rewrite routes in exception handler

[ms-afk]

The issue

If a rewrite route is defined in an exception handler, Router's method handleException will, currently, be adding that route to the processedRoutes array without removing the hasPendingRewrite flag. This leads to the associated callback being executed twice if the callback itself returns NULL. This happens because the handleRouteRewrite method, finding that hasPendingRewrite is still set to true, adds the rewriteRoute to the processedRoutes for a second time, before finally setting that flag to false. This should not happen because executing code twice might have unexpected consequences.

How to avoid the issue in previous versions

• The first option is to return something different from NULL in the callback. This will stop the execution of the "outer routeRequest call" after the "inner one" has been executed, effectively limiting the callback executions to only one.

• A second option is to define and use a custom class loader that casts callbacks's return values to string. This way, when a NULL value will be returned by a callback, the value will be transformed into an empty string "", which is !==NULL, and, for the reasons mentioned in the previous point, this will lead to a normal single execution of the callback.

UnitTest

I wrote a UnitTest to show the issue. I also defined a custom class loader in the file because the current default one always casts return values to string, making the bug disappear (see the "How to aviod the issue in previous versions" section to understand why). It should be noted that the class loader interface IClassLoader doesn't pose any restrictions on the return type of the loadClassMethod and loadClosure methods, so in general the bug can easily manifest if the default loader implementation gets changed or if a custom loader is used.

<?php

use Pecee\Http\Request;
use Pecee\SimpleRouter\Exceptions\NotFoundHttpException;
use Pecee\SimpleRouter\ClassLoader\IClassLoader;

class RewriteCallbackInsideErrorHandlerTest extends \PHPUnit\Framework\TestCase
{
    public function testRewriteCallbackInsideErrorHandler()
    {
        TestRouter::setCustomClassLoader(new MyCustomClassLoader());

        TestRouter::error(function(Request $request, \Exception $exception) {
            if($exception instanceof NotFoundHttpException && $exception->getCode() === 404) {
                $request->setRewriteCallback(static function() {
                    echo('executed');
                });
            }
        });

        $result = TestRouter::debugOutput('/non-existent-route', 'get');
        $this->assertEquals('executed', $result);
    }
}

/**
 * Custom ClassLoader whose loadClassMethod and loadClosure methods can return NULL.
 */
class MyCustomClassLoader implements IClassLoader
{
    public function loadClass(string $class)
    {
        if (\class_exists($class) === false) {
            throw new NotFoundHttpException(sprintf('Class "%s" does not exist', $class), 404);
        }

        return new $class();
    }

    public function loadClassMethod($class, string $method, array $parameters)
    {
        return call_user_func_array([$class, $method], array_values($parameters));
    }

    public function loadClosure(Callable $closure, array $parameters)
    {
        return \call_user_func_array($closure, array_values($parameters));
    }

}

You'll see that in the current version of the library the test will fail because the string 'executed' will be printed twice. After the fix, the test will instead pass. All other UnitTests also are passing after the change.

[skipperbent]

Very detailed walkthrough, thanks so much and superb fix. Thanks for the contribution brother