From 788ddb6137e17eb40d3b26f09a96aed8da5a1f89 Mon Sep 17 00:00:00 2001 From: Austin Collier Date: Tue, 20 Feb 2024 10:46:46 -0700 Subject: [PATCH 1/5] added streaming responses. Fixed JSONP. --- .gitignore | 2 -- .vscode/settings.json | 5 ++++ flight/Engine.php | 48 ++++++++++++++++++++----------- flight/net/Response.php | 8 ++++++ flight/net/Route.php | 28 ++++++++++++++++++ tests/EngineTest.php | 32 +++++++++++++++++++++ tests/FlightTest.php | 26 +++++++++++++++++ tests/run_all_tests.sh | 9 ++++++ tests/server-v2/index.php | 24 ++++++++++------ tests/server/LayoutMiddleware.php | 2 ++ tests/server/index.php | 24 +++++++++++++--- 11 files changed, 178 insertions(+), 30 deletions(-) create mode 100644 .vscode/settings.json create mode 100644 tests/run_all_tests.sh diff --git a/.gitignore b/.gitignore index fd379ad..9bed37c 100644 --- a/.gitignore +++ b/.gitignore @@ -4,7 +4,5 @@ composer.phar composer.lock .phpunit.result.cache coverage/ -.vscode/settings.json *.sublime* -.vscode/ clover.xml diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..2e9942f --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,5 @@ +{ + "php.suggest.basic": false, + "editor.detectIndentation": false, + "editor.insertSpaces": true +} \ No newline at end of file diff --git a/flight/Engine.php b/flight/Engine.php index 8dbb6dc..eda4c52 100644 --- a/flight/Engine.php +++ b/flight/Engine.php @@ -364,15 +364,17 @@ class Engine /** * Processes each routes middleware. * - * @param array $middleware Middleware attached to the route. - * @param array $params `$route->params`. + * @param Route $route The route to process the middleware for. * @param string $event_name If this is the before or after method. */ - protected function processMiddleware(array $middleware, array $params, string $event_name): bool + protected function processMiddleware(Route $route, string $event_name): bool { $at_least_one_middleware_failed = false; - foreach ($middleware as $middleware) { + $middlewares = $event_name === 'before' ? $route->middleware : array_reverse($route->middleware); + $params = $route->params; + + foreach ($middlewares as $middleware) { $middleware_object = false; if ($event_name === 'before') { @@ -399,14 +401,14 @@ class Engine continue; } - if ($this->response()->v2_output_buffering === false) { + if ($this->response()->v2_output_buffering === false && $route->is_streamed === 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) { + if ($this->response()->v2_output_buffering === false && $route->is_streamed === false) { $this->response()->write(ob_get_clean()); } @@ -462,16 +464,32 @@ class Engine $params[] = $route; } + // If this route is to be streamed, we need to output the headers now + if ($route->is_streamed === true) { + $response->status($route->streamed_headers['status']); + unset($route->streamed_headers['status']); + $response->header('X-Accel-Buffering', 'no'); + $response->header('Connection', 'close'); + foreach ($route->streamed_headers as $header => $value) { + $response->header($header, $value); + } + + // We obviously don't know the content length right now. This must be false. + $response->content_length = false; + $response->sendHeaders(); + $response->markAsSent(); + } + // Run any before middlewares if (count($route->middleware) > 0) { - $at_least_one_middleware_failed = $this->processMiddleware($route->middleware, $route->params, 'before'); + $at_least_one_middleware_failed = $this->processMiddleware($route, 'before'); if ($at_least_one_middleware_failed === true) { $failed_middleware_check = true; break; } } - if ($response->v2_output_buffering === false) { + if ($response->v2_output_buffering === false && $route->is_streamed === false) { ob_start(); } @@ -481,18 +499,14 @@ class Engine $params ); - if ($response->v2_output_buffering === false) { + if ($response->v2_output_buffering === false && $route->is_streamed === false) { $response->write(ob_get_clean()); } // Run any before middlewares if (count($route->middleware) > 0) { // process the middleware in reverse order now - $at_least_one_middleware_failed = $this->processMiddleware( - array_reverse($route->middleware), - $route->params, - 'after' - ); + $at_least_one_middleware_failed = $this->processMiddleware($route, 'after'); if ($at_least_one_middleware_failed === true) { $failed_middleware_check = true; @@ -774,8 +788,10 @@ class Engine $this->response() ->status($code) ->header('Content-Type', 'application/javascript; charset=' . $charset) - ->write($callback . '(' . $json . ');') - ->send(); + ->write($callback . '(' . $json . ');'); + if ($this->response()->v2_output_buffering === true) { + $this->response()->send(); + } } /** diff --git a/flight/net/Response.php b/flight/net/Response.php index 761d1a6..cfc8ffd 100644 --- a/flight/net/Response.php +++ b/flight/net/Response.php @@ -386,6 +386,14 @@ class Response return $this->sent; } + /** + * Marks the response as sent. + */ + public function markAsSent(): void + { + $this->sent = true; + } + /** * Sends a HTTP response. */ diff --git a/flight/net/Route.php b/flight/net/Route.php index 2070cc6..480ff7c 100644 --- a/flight/net/Route.php +++ b/flight/net/Route.php @@ -67,6 +67,20 @@ class Route */ public array $middleware = []; + /** + * Whether the response for this route should be streamed. + * + * @var boolean + */ + public bool $is_streamed = false; + + /** + * If this route is streamed, the headers to be sent before the response. + * + * @var array + */ + public array $streamed_headers = []; + /** * Constructor. * @@ -225,4 +239,18 @@ class Route } return $this; } + + /** + * This will allow the response for this route to be streamed. + * + * @param array $headers a key value of headers to set before the stream starts. + * + * @return self + */ + public function streamWithHeaders(array $headers): self + { + $this->is_streamed = true; + $this->streamed_headers = $headers; + return $this; + } } diff --git a/tests/EngineTest.php b/tests/EngineTest.php index 1f9d456..c7e21d2 100644 --- a/tests/EngineTest.php +++ b/tests/EngineTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace tests; use Exception; +use Flight; use flight\Engine; use flight\net\Response; use PHPUnit\Framework\TestCase; @@ -307,6 +308,16 @@ class EngineTest extends TestCase { $engine = new Engine(); $engine->json(['key1' => 'value1', 'key2' => 'value2']); + $this->assertEquals('application/json; charset=utf-8', $engine->response()->headers()['Content-Type']); + $this->assertEquals(200, $engine->response()->status()); + $this->assertEquals('{"key1":"value1","key2":"value2"}', $engine->response()->getBody()); + } + + public function testJsonV2OutputBuffering() + { + $engine = new Engine(); + $engine->response()->v2_output_buffering = true; + $engine->json(['key1' => 'value1', 'key2' => 'value2']); $this->expectOutputString('{"key1":"value1","key2":"value2"}'); $this->assertEquals('application/json; charset=utf-8', $engine->response()->headers()['Content-Type']); $this->assertEquals(200, $engine->response()->status()); @@ -317,6 +328,17 @@ class EngineTest extends TestCase $engine = new Engine(); $engine->request()->query['jsonp'] = 'whatever'; $engine->jsonp(['key1' => 'value1', 'key2' => 'value2']); + $this->assertEquals('application/javascript; charset=utf-8', $engine->response()->headers()['Content-Type']); + $this->assertEquals(200, $engine->response()->status()); + $this->assertEquals('whatever({"key1":"value1","key2":"value2"});', $engine->response()->getBody()); + } + + public function testJsonPV2OutputBuffering() + { + $engine = new Engine(); + $engine->response()->v2_output_buffering = true; + $engine->request()->query['jsonp'] = 'whatever'; + $engine->jsonp(['key1' => 'value1', 'key2' => 'value2']); $this->expectOutputString('whatever({"key1":"value1","key2":"value2"});'); $this->assertEquals('application/javascript; charset=utf-8', $engine->response()->headers()['Content-Type']); $this->assertEquals(200, $engine->response()->status()); @@ -326,6 +348,16 @@ class EngineTest extends TestCase { $engine = new Engine(); $engine->jsonp(['key1' => 'value1', 'key2' => 'value2']); + $this->assertEquals('({"key1":"value1","key2":"value2"});', $engine->response()->getBody()); + $this->assertEquals('application/javascript; charset=utf-8', $engine->response()->headers()['Content-Type']); + $this->assertEquals(200, $engine->response()->status()); + } + + public function testJsonpBadParamV2OutputBuffering() + { + $engine = new Engine(); + $engine->response()->v2_output_buffering = true; + $engine->jsonp(['key1' => 'value1', 'key2' => 'value2']); $this->expectOutputString('({"key1":"value1","key2":"value2"});'); $this->assertEquals('application/javascript; charset=utf-8', $engine->response()->headers()['Content-Type']); $this->assertEquals(200, $engine->response()->status()); diff --git a/tests/FlightTest.php b/tests/FlightTest.php index c068dbe..a6ffa16 100644 --- a/tests/FlightTest.php +++ b/tests/FlightTest.php @@ -278,4 +278,30 @@ class FlightTest extends TestCase Flight::start(); $this->assertEquals('hooked before starttest', Flight::response()->getBody()); } + + public function testStreamRoute() + { + $response_mock = new class extends Response { + public function setRealHeader(string $header_string, bool $replace = true, int $response_code = 0): Response + { + return $this; + } + }; + $mock_response_class_name = get_class($response_mock); + Flight::register('response', $mock_response_class_name); + Flight::route('/stream', function () { + echo 'stream'; + })->streamWithHeaders(['Content-Type' => 'text/plain', 'X-Test' => 'test', 'status' => 200 ]); + Flight::request()->url = '/stream'; + $this->expectOutputString('stream'); + Flight::start(); + $this->assertEquals('', Flight::response()->getBody()); + $this->assertEquals([ + 'Content-Type' => 'text/plain', + 'X-Test' => 'test', + 'X-Accel-Buffering' => 'no', + 'Connection' => 'close' + ], Flight::response()->getHeaders()); + $this->assertEquals(200, Flight::response()->status()); + } } diff --git a/tests/run_all_tests.sh b/tests/run_all_tests.sh new file mode 100644 index 0000000..d38bf0d --- /dev/null +++ b/tests/run_all_tests.sh @@ -0,0 +1,9 @@ +#!/bin/bash + +# Run all tests +composer lint +composer beautify +composer phpcs +composer test-coverage +xdg-open http://localhost:8000 +composer test-server \ No newline at end of file diff --git a/tests/server-v2/index.php b/tests/server-v2/index.php index 3eb765f..d4cc385 100644 --- a/tests/server-v2/index.php +++ b/tests/server-v2/index.php @@ -20,9 +20,9 @@ Flight::set('flight.v2.output_buffering', true); // Test 1: Root route Flight::route('/', function () { echo 'Route text: Root route works!'; - if (Flight::request()->query->redirected) { - echo '
Redirected from /redirect route successfully!'; - } + if (Flight::request()->query->redirected) { + echo '
Redirected from /redirect route successfully!'; + } }); Flight::route('/querytestpath', function () { echo 'Route text: This ir query route
'; @@ -99,24 +99,31 @@ Flight::route('/template/@name', function ($name) { // Test 8: Throw an error Flight::route('/error', function () { - trigger_error('This is a successful error'); + trigger_error('This is a successful error'); }); // Test 9: JSON output (should not output any other html) Flight::route('/json', function () { - echo "\n\n\n\n\n"; + echo "\n\n\n\n\n"; Flight::json(['message' => 'JSON renders successfully!']); - echo "\n\n\n\n\n"; + echo "\n\n\n\n\n"; +}); + +// Test 13: JSONP output (should not output any other html) +Flight::route('/jsonp', function () { + echo "\n\n\n\n\n"; + Flight::jsonp(['message' => 'JSONP renders successfully!'], 'jsonp'); + echo "\n\n\n\n\n"; }); // Test 10: Halt Flight::route('/halt', function () { - Flight::halt(400, 'Halt worked successfully'); + Flight::halt(400, 'Halt worked successfully'); }); // Test 11: Redirect Flight::route('/redirect', function () { - Flight::redirect('/?redirected=1'); + Flight::redirect('/?redirected=1'); }); Flight::set('flight.views.path', './'); @@ -192,6 +199,7 @@ echo '
  • Mega group
  • Error
  • JSON
    • +
  • JSONP
  • Halt
  • Redirect
    • '; diff --git a/tests/server/LayoutMiddleware.php b/tests/server/LayoutMiddleware.php index 7be2cda..d5dfcde 100644 --- a/tests/server/LayoutMiddleware.php +++ b/tests/server/LayoutMiddleware.php @@ -73,8 +73,10 @@ class LayoutMiddleware
  • Mega group
  • Error
  • JSON
    • +
  • JSONP
  • Halt
  • Redirect
    • +
  • Stream
    • HTML; echo '
    '; diff --git a/tests/server/index.php b/tests/server/index.php index 30c1176..e3ea703 100644 --- a/tests/server/index.php +++ b/tests/server/index.php @@ -104,10 +104,21 @@ Flight::group('', function () { Flight::halt(400, 'Halt worked successfully'); }); - // Test 11: Redirect - Flight::route('/redirect', function () { - Flight::redirect('/?redirected=1'); - }); + // Test 11: Redirect + Flight::route('/redirect', function () { + Flight::redirect('/?redirected=1'); + }); + + // Test 12: Redirect with status code + Flight::route('/streamResponse', function () { + echo "Streaming a response"; + for ($i = 1; $i <= 50; $i++) { + echo "."; + usleep(50000); + ob_flush(); + } + echo "is successful!!"; + })->streamWithHeaders(['Content-Type' => 'text/html', 'status' => 200 ]); }, [ new LayoutMiddleware() ]); // Test 9: JSON output (should not output any other html) @@ -115,6 +126,11 @@ Flight::route('/json', function () { Flight::json(['message' => 'JSON renders successfully!']); }); +// Test 13: JSONP output (should not output any other html) +Flight::route('/jsonp', function () { + Flight::jsonp(['message' => 'JSONP renders successfully!'], 'jsonp'); +}); + Flight::map('error', function (Throwable $e) { echo sprintf( '

    500 Internal Server Error

    ' . From 7887b33e07a8b92c308f421a794bbcf033c58ed2 Mon Sep 17 00:00:00 2001 From: fadrian06 Date: Tue, 20 Feb 2024 23:21:44 -0400 Subject: [PATCH 2/5] Spaces my friend, spaces... --- .vscode/settings.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 2e9942f..fcf56e7 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,5 +1,5 @@ { - "php.suggest.basic": false, - "editor.detectIndentation": false, - "editor.insertSpaces": true -} \ No newline at end of file + "php.suggest.basic": false, + "editor.detectIndentation": false, + "editor.insertSpaces": true +} From c40b0b775947df790c02d1b7bbd3fe89d0abdc95 Mon Sep 17 00:00:00 2001 From: fadrian06 Date: Tue, 20 Feb 2024 23:22:09 -0400 Subject: [PATCH 3/5] Simplified docblocks --- flight/net/Route.php | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/flight/net/Route.php b/flight/net/Route.php index 480ff7c..baf5ea1 100644 --- a/flight/net/Route.php +++ b/flight/net/Route.php @@ -63,15 +63,11 @@ class Route /** * The middleware to be applied to the route * - * @var array + * @var array */ public array $middleware = []; - /** - * Whether the response for this route should be streamed. - * - * @var boolean - */ + /** Whether the response for this route should be streamed. */ public bool $is_streamed = false; /** @@ -193,7 +189,7 @@ class Route /** * Hydrates the route url with the given parameters * - * @param array $params the parameters to pass to the route + * @param array $params the parameters to pass to the route */ public function hydrateUrl(array $params = []): string { @@ -226,9 +222,7 @@ class Route /** * Sets the route middleware * - * @param array|callable $middleware - * - * @return self + * @param array|callable $middleware */ public function addMiddleware($middleware): self { @@ -245,12 +239,13 @@ class Route * * @param array $headers a key value of headers to set before the stream starts. * - * @return self + * @return $this */ public function streamWithHeaders(array $headers): self { $this->is_streamed = true; $this->streamed_headers = $headers; + return $this; } } From 090a6f1922729513cdd503e894ce4486a9b7c2a1 Mon Sep 17 00:00:00 2001 From: Austin Collier Date: Tue, 20 Feb 2024 22:05:18 -0700 Subject: [PATCH 4/5] few minor changes from code review --- flight/Engine.php | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/flight/Engine.php b/flight/Engine.php index eda4c52..fb4d9e8 100644 --- a/flight/Engine.php +++ b/flight/Engine.php @@ -371,29 +371,29 @@ class Engine { $at_least_one_middleware_failed = false; - $middlewares = $event_name === 'before' ? $route->middleware : array_reverse($route->middleware); + $middlewares = $event_name === Dispatcher::FILTER_BEFORE ? $route->middleware : array_reverse($route->middleware); $params = $route->params; foreach ($middlewares as $middleware) { $middleware_object = false; - if ($event_name === 'before') { + if ($event_name === Dispatcher::FILTER_BEFORE) { // can be a callable or a class $middleware_object = (is_callable($middleware) === true ? $middleware - : (method_exists($middleware, 'before') === true - ? [$middleware, 'before'] + : (method_exists($middleware, Dispatcher::FILTER_BEFORE) === true + ? [$middleware, Dispatcher::FILTER_BEFORE] : false ) ); - } elseif ($event_name === 'after') { + } elseif ($event_name === Dispatcher::FILTER_AFTER) { // must be an object. No functions allowed here if ( is_object($middleware) === true && !($middleware instanceof Closure) - && method_exists($middleware, 'after') === true + && method_exists($middleware, Dispatcher::FILTER_AFTER) === true ) { - $middleware_object = [$middleware, 'after']; + $middleware_object = [$middleware, Dispatcher::FILTER_AFTER]; } } @@ -401,14 +401,18 @@ class Engine continue; } - if ($this->response()->v2_output_buffering === false && $route->is_streamed === false) { + $use_v3_output_buffering = + $this->response()->v2_output_buffering === false && + $route->is_streamed === false; + + if ($use_v3_output_buffering === true) { 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 && $route->is_streamed === false) { + if ($use_v3_output_buffering === true) { $this->response()->write(ob_get_clean()); } @@ -489,7 +493,11 @@ class Engine } } - if ($response->v2_output_buffering === false && $route->is_streamed === false) { + $use_v3_output_buffering = + $this->response()->v2_output_buffering === false && + $route->is_streamed === false; + + if ($use_v3_output_buffering === true) { ob_start(); } @@ -499,7 +507,7 @@ class Engine $params ); - if ($response->v2_output_buffering === false && $route->is_streamed === false) { + if ($use_v3_output_buffering === true) { $response->write(ob_get_clean()); } From 65c4509297649cbd51d95301d2aa5b37cb5fc171 Mon Sep 17 00:00:00 2001 From: fadrian06 Date: Tue, 20 Feb 2024 23:31:17 -0400 Subject: [PATCH 5/5] Added phpstan baseline --- .gitattributes | 1 + phpstan-baseline.neon | 6 ++++++ phpstan.neon | 1 + 3 files changed, 8 insertions(+) create mode 100644 phpstan-baseline.neon diff --git a/.gitattributes b/.gitattributes index d90daf1..0c5892c 100644 --- a/.gitattributes +++ b/.gitattributes @@ -5,4 +5,5 @@ /.gitignore export-ignore /phpcs.xml export-ignore /phpstan.neon export-ignore +/phpstan-baseline.neon export-ignore /phpunit.xml export-ignore diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon new file mode 100644 index 0000000..9fa597c --- /dev/null +++ b/phpstan-baseline.neon @@ -0,0 +1,6 @@ +parameters: + ignoreErrors: + - + message: "#^Parameter \\#2 \\$callback of method flight\\\\core\\\\Dispatcher\\:\\:set\\(\\) expects Closure\\(\\)\\: mixed, array\\{\\$this\\(flight\\\\Engine\\), literal\\-string&non\\-falsy\\-string\\} given\\.$#" + count: 1 + path: flight/Engine.php diff --git a/phpstan.neon b/phpstan.neon index 95afff0..97e16eb 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,5 +1,6 @@ includes: - vendor/phpstan/phpstan/conf/bleedingEdge.neon + - phpstan-baseline.neon parameters: level: 6