diff --git a/flight/Engine.php b/flight/Engine.php index 58b0859..638c6b8 100644 --- a/flight/Engine.php +++ b/flight/Engine.php @@ -15,6 +15,7 @@ use flight\net\Router; use flight\template\View; use Throwable; use flight\net\Route; +use PHPUnit\Framework\TestCase; /** * The Engine class contains the core functionality of the framework. @@ -27,7 +28,7 @@ use flight\net\Route; * # Core methods * @method void start() Starts engine * @method void stop() Stops framework and outputs current response - * @method void halt(int $code = 200, string $message = '') Stops processing and returns a given response. + * @method void halt(int $code = 200, string $message = '', bool $actually_exit = true) Stops processing and returns a given response. * * # Routing * @method Route route(string $pattern, callable $callback, bool $pass_route = false, string $alias = '') @@ -156,6 +157,7 @@ class Engine $this->set('flight.views.path', './views'); $this->set('flight.views.extension', '.php'); $this->set('flight.content_length', true); + $this->set('flight.v2.output_buffering', false); // Startup configuration $this->before('start', function () use ($self) { @@ -169,6 +171,10 @@ class Engine $self->router()->case_sensitive = $self->get('flight.case_sensitive'); // Set Content-Length $self->response()->content_length = $self->get('flight.content_length'); + // This is to maintain legacy handling of output buffering + // which causes a lot of problems. This will be removed + // in v4 + $self->response()->v2_output_buffering = $this->get('flight.v2.output_buffering'); }); $this->initialized = true; @@ -354,7 +360,67 @@ class Engine $this->loader->addDirectory($dir); } - // Extensible Methods + /** + * Processes each routes middleware + * + * @param array $middleware middleware attached to the route + * @param array $params route->params + * @param string $event_name if this is the before or after method + * + * @return boolean + */ + protected function processMiddleware(array $middleware, array $params, string $event_name): bool + { + $at_least_one_middleware_failed = false; + foreach ($middleware as $middleware) { + $middleware_object = false; + + if ($event_name === 'before') { + // can be a callable or a class + $middleware_object = (is_callable($middleware) === true + ? $middleware + : (method_exists($middleware, 'before') === true + ? [$middleware, 'before'] + : false)); + } elseif ($event_name === 'after') { + // must be an object. No functions allowed here + if ( + is_object($middleware) === true + && !($middleware instanceof Closure) + && method_exists($middleware, 'after') === true + ) { + $middleware_object = [$middleware, 'after']; + } + } + + if ($middleware_object === false) { + continue; + } + + if ($this->response()->v2_output_buffering === false) { + ob_start(); + } + + // It's assumed if you don't declare before, that it will be assumed as the before method + $middleware_result = $middleware_object($params); + + if ($this->response()->v2_output_buffering === false) { + $this->response()->write(ob_get_clean()); + } + + if ($middleware_result === false) { + $at_least_one_middleware_failed = true; + break; + } + } + + return $at_least_one_middleware_failed; + } + + /********************************* + * + * Extensible Methods + *********************************/ /** * Starts the framework. @@ -374,13 +440,16 @@ class Engine $self->stop(); }); - // Flush any existing output - if (ob_get_length() > 0) { - $response->write(ob_get_clean()); // @codeCoverageIgnore - } + if ($response->v2_output_buffering === true) { + // Flush any existing output + if (ob_get_length() > 0) { + $response->write(ob_get_clean()); // @codeCoverageIgnore + } - // Enable output buffering - ob_start(); + // Enable output buffering + // This is closed in the Engine->_stop() method + ob_start(); + } // Route the request $failed_middleware_check = false; @@ -394,60 +463,39 @@ class Engine // Run any before middlewares if (count($route->middleware) > 0) { - foreach ($route->middleware as $middleware) { - $middleware_object = (is_callable($middleware) === true - ? $middleware - : (method_exists($middleware, 'before') === true - ? [$middleware, 'before'] - : false)); - - if ($middleware_object === false) { - continue; - } - - // It's assumed if you don't declare before, that it will be assumed as the before method - $middleware_result = $middleware_object($route->params); - - if ($middleware_result === false) { - $failed_middleware_check = true; - break 2; - } + $at_least_one_middleware_failed = $this->processMiddleware($route->middleware, $route->params, 'before'); + if ($at_least_one_middleware_failed === true) { + $failed_middleware_check = true; + break; } } + if ($response->v2_output_buffering === false) { + ob_start(); + } + // Call route handler $continue = $this->dispatcher->execute( $route->callback, $params ); + if ($response->v2_output_buffering === false) { + $response->write(ob_get_clean()); + } // Run any before middlewares if (count($route->middleware) > 0) { // process the middleware in reverse order now - foreach (array_reverse($route->middleware) as $middleware) { - // must be an object. No functions allowed here - $middleware_object = false; - - if ( - is_object($middleware) === true - && !($middleware instanceof Closure) - && method_exists($middleware, 'after') === true - ) { - $middleware_object = [$middleware, 'after']; - } - - // has to have the after method, otherwise just skip it - if ($middleware_object === false) { - continue; - } - - $middleware_result = $middleware_object($route->params); - - if ($middleware_result === false) { - $failed_middleware_check = true; - break 2; - } + $at_least_one_middleware_failed = $this->processMiddleware( + array_reverse($route->middleware), + $route->params, + 'after' + ); + + if ($at_least_one_middleware_failed === true) { + $failed_middleware_check = true; + break; } } @@ -463,7 +511,7 @@ class Engine } if ($failed_middleware_check === true) { - $this->halt(403, 'Forbidden'); + $this->halt(403, 'Forbidden', empty(getenv('PHPUNIT_TEST'))); } elseif ($dispatched === false) { $this->notFound(); } @@ -514,8 +562,9 @@ class Engine $response->status($code); } - $content = ob_get_clean(); - $response->write($content ?: ''); + if ($response->v2_output_buffering === true && ob_get_length() > 0) { + $response->write(ob_get_clean()); + } $response->send(); } @@ -599,16 +648,16 @@ class Engine * * @param int $code HTTP status code * @param string $message Response message + * @param bool $actually_exit Whether to actually exit the script or just send response */ - public function _halt(int $code = 200, string $message = ''): void + public function _halt(int $code = 200, string $message = '', bool $actually_exit = true): void { $this->response() ->clear() ->status($code) ->write($message) ->send(); - // apologies for the crappy hack here... - if ($message !== 'skip---exit') { + if ($actually_exit === true) { exit(); // @codeCoverageIgnore } } @@ -742,7 +791,7 @@ class Engine isset($_SERVER['HTTP_IF_NONE_MATCH']) && $_SERVER['HTTP_IF_NONE_MATCH'] === $id ) { - $this->halt(304); + $this->halt(304, '', empty(getenv('PHPUNIT_TEST'))); } } @@ -759,7 +808,7 @@ class Engine isset($_SERVER['HTTP_IF_MODIFIED_SINCE']) && strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']) === $time ) { - $this->halt(304); + $this->halt(304, '', empty(getenv('PHPUNIT_TEST'))); } } diff --git a/flight/Flight.php b/flight/Flight.php index c53b979..955ee87 100644 --- a/flight/Flight.php +++ b/flight/Flight.php @@ -22,7 +22,7 @@ require_once __DIR__ . '/autoload.php'; * @method static void start() Starts the framework. * @method static void path(string $path) Adds a path for autoloading classes. * @method static void stop(?int $code = null) Stops the framework and sends a response. - * @method static void halt(int $code = 200, string $message = '') + * @method static void halt(int $code = 200, string $message = '', bool $actually_exit = true) * Stop the framework with an optional status code and message. * * # Routing diff --git a/flight/net/Response.php b/flight/net/Response.php index 55e2605..61df314 100644 --- a/flight/net/Response.php +++ b/flight/net/Response.php @@ -21,6 +21,15 @@ class Response */ public bool $content_length = true; + /** + * This is to maintain legacy handling of output buffering + * which causes a lot of problems. This will be removed + * in v4 + * + * @var boolean + */ + public bool $v2_output_buffering = false; + /** * HTTP status codes * @@ -96,6 +105,7 @@ class Response 510 => 'Not Extended', 511 => 'Network Authentication Required', ]; + /** * HTTP status */ @@ -198,6 +208,11 @@ class Response $this->headers = []; $this->body = ''; + // This needs to clear the output buffer if it's on + if ($this->v2_output_buffering === false && ob_get_length() > 0) { + ob_clean(); + } + return $this; } @@ -338,8 +353,11 @@ class Response */ public function send(): void { - if (ob_get_length() > 0) { - ob_end_clean(); // @codeCoverageIgnore + // legacy way of handling this + if ($this->v2_output_buffering === true) { + if (ob_get_length() > 0) { + ob_end_clean(); // @codeCoverageIgnore + } } if (!headers_sent()) { diff --git a/phpunit.xml b/phpunit.xml index 2fedaf9..9805e7b 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -22,4 +22,7 @@ + + + diff --git a/tests/DocExamplesTest.php b/tests/DocExamplesTest.php index 76bbb21..dd5d6c6 100644 --- a/tests/DocExamplesTest.php +++ b/tests/DocExamplesTest.php @@ -40,7 +40,26 @@ class DocExamplesTest extends TestCase }); Flight::start(); - ob_get_clean(); + $this->expectOutputString('[]'); + $this->assertEquals(404, Flight::response()->status()); + $this->assertEquals('[]', Flight::response()->getBody()); + } + + public function testMapNotFoundMethodV2OutputBuffering() + { + Flight::map('notFound', function () { + Flight::json([], 404); + }); + + Flight::request()->url = '/not-found'; + + Flight::route('/', function () { + echo 'hello world!'; + }); + + Flight::set('flight.v2.output_buffering', true); + Flight::start(); + ob_get_clean(); $this->assertEquals(404, Flight::response()->status()); $this->assertEquals('[]', Flight::response()->getBody()); } diff --git a/tests/EngineTest.php b/tests/EngineTest.php index 0a20b53..1f9d456 100644 --- a/tests/EngineTest.php +++ b/tests/EngineTest.php @@ -30,11 +30,31 @@ class EngineTest extends TestCase return $this->initialized; } }; + $this->assertTrue($engine->getInitializedVar()); + + // we need to setup a dummy route + $engine->route('/someRoute', function () { }); + $engine->request()->url = '/someRoute'; + $engine->start(); + + $this->assertFalse($engine->router()->case_sensitive); + $this->assertTrue($engine->response()->content_length); + } + + public function testInitBeforeStartV2OutputBuffering() + { + $engine = new class extends Engine { + public function getInitializedVar() + { + return $this->initialized; + } + }; + $engine->set('flight.v2.output_buffering', true); $this->assertTrue($engine->getInitializedVar()); $engine->start(); - // this is necessary cause it doesn't actually send the response correctly - ob_end_clean(); + // This is a necessary evil because of how the v2 output buffer works. + ob_end_clean(); $this->assertFalse($engine->router()->case_sensitive); $this->assertTrue($engine->response()->content_length); @@ -126,6 +146,26 @@ class EngineTest extends TestCase $this->expectOutputString('

404 Not Found

The page you have requested could not be found.

'); $engine->start(); } + + public function testStartWithRouteButReturnedValueThrows404V2OutputBuffering() + { + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_SERVER['REQUEST_URI'] = '/someRoute'; + + $engine = new class extends Engine { + public function getInitializedVar() + { + return $this->initialized; + } + }; + $engine->set('flight.v2.output_buffering', true); + $engine->route('/someRoute', function () { + echo 'i ran'; + return true; + }, true); + $this->expectOutputString('

404 Not Found

The page you have requested could not be found.

'); + $engine->start(); + } public function testStopWithCode() { @@ -144,14 +184,40 @@ class EngineTest extends TestCase } }; }); - // need to add another one of these because _stop() stops and gets clean, but $response->send() does too..... - ob_start(); $engine->response()->write('I am a teapot'); $this->expectOutputString('I am a teapot'); $engine->stop(500); $this->assertEquals(500, $engine->response()->status()); } + public function testStopWithCodeV2OutputBuffering() + { + $engine = new class extends Engine { + public function getLoader() + { + return $this->loader; + } + }; + // doing this so we can overwrite some parts of the response + $engine->getLoader()->register('response', function () { + return new class extends Response { + public function setRealHeader(string $header_string, bool $replace = true, int $response_code = 0): self + { + return $this; + } + }; + }); + $engine->set('flight.v2.output_buffering', true); + $engine->route('/testRoute', function () use ($engine) { + echo 'I am a teapot'; + $engine->stop(500); + }); + $engine->request()->url = '/testRoute'; + $engine->start(); + $this->expectOutputString('I am a teapot'); + $this->assertEquals(500, $engine->response()->status()); + } + public function testPostRoute() { $engine = new Engine(); @@ -207,10 +273,6 @@ class EngineTest extends TestCase // doing this so we can overwrite some parts of the response $engine->getLoader()->register('response', function () { return new class extends Response { - public function __construct() - { - } - public function setRealHeader( string $header_string, bool $replace = true, @@ -220,9 +282,7 @@ class EngineTest extends TestCase } }; }); - - $this->expectOutputString('skip---exit'); - $engine->halt(500, 'skip---exit'); + $engine->halt(500, '', false); $this->assertEquals(500, $engine->response()->status()); } @@ -280,17 +340,10 @@ class EngineTest extends TestCase public function testEtagWithHttpIfNoneMatch() { - // just need this not to exit... - $engine = new class extends Engine { - public function _halt(int $code = 200, string $message = ''): void - { - $this->response()->status($code); - $this->response()->write($message); - } - }; + $engine = new Engine; $_SERVER['HTTP_IF_NONE_MATCH'] = 'etag'; $engine->etag('etag'); - $this->assertEquals('"etag"', $engine->response()->headers()['ETag']); + $this->assertTrue(empty($engine->response()->headers()['ETag'])); $this->assertEquals(304, $engine->response()->status()); } @@ -303,17 +356,10 @@ class EngineTest extends TestCase public function testLastModifiedWithHttpIfModifiedSince() { - // just need this not to exit... - $engine = new class extends Engine { - public function _halt(int $code = 200, string $message = ''): void - { - $this->response()->status($code); - $this->response()->write($message); - } - }; + $engine = new Engine; $_SERVER['HTTP_IF_MODIFIED_SINCE'] = 'Fri, 13 Feb 2009 23:31:30 GMT'; $engine->lastModified(1234567890); - $this->assertEquals('Fri, 13 Feb 2009 23:31:30 GMT', $engine->response()->headers()['Last-Modified']); + $this->assertTrue(empty($engine->response()->headers()['Last-Modified'])); $this->assertEquals(304, $engine->response()->status()); } @@ -344,11 +390,6 @@ class EngineTest extends TestCase public function testMiddlewareCallableFunctionReturnFalse() { $engine = new class extends Engine { - public function _halt(int $code = 200, string $message = ''): void - { - $this->response()->status($code); - $this->response()->write($message); - } }; $engine->route('/path1/@id', function ($id) { echo 'OK' . $id; @@ -359,7 +400,7 @@ class EngineTest extends TestCase }); $engine->request()->url = '/path1/123'; $engine->start(); - $this->expectOutputString('Forbiddenbefore123'); + $this->expectOutputString('Forbidden'); $this->assertEquals(403, $engine->response()->status()); } @@ -410,7 +451,6 @@ class EngineTest extends TestCase $middleware = new class { public function after($params) { - echo 'after' . $params['id']; } }; @@ -435,11 +475,6 @@ class EngineTest extends TestCase } }; $engine = new class extends Engine { - public function _halt(int $code = 200, string $message = ''): void - { - $this->response()->status($code); - $this->response()->write($message); - } }; $engine->route('/path1/@id', function ($id) { @@ -449,7 +484,7 @@ class EngineTest extends TestCase $engine->request()->url = '/path1/123'; $engine->start(); $this->assertEquals(403, $engine->response()->status()); - $this->expectOutputString('ForbiddenOK123after123'); + $this->expectOutputString('Forbidden'); } public function testMiddlewareCallableFunctionMultiple()