Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output buffer middleware #2166

Merged
merged 3 commits into from
Jun 27, 2017
Merged

Conversation

codeguy
Copy link
Member

@codeguy codeguy commented Mar 8, 2017

Work in progress. Opening PR for feedback.

@codeguy codeguy added this to the 4.0 milestone Mar 8, 2017
@codeguy
Copy link
Member Author

codeguy commented Mar 8, 2017

Changes so far include:

  1. Require all route callbacks to return PSR7 response.
  2. Remove all output buffering concerns from App and Route classes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 96.693% when pulling 9f6e225 on codeguy:feature-output-buffer-middleware into 724459a on slimphp:4.x.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 96.613% when pulling a10b833 on codeguy:feature-output-buffer-middleware into 724459a on slimphp:4.x.

@codeguy codeguy changed the title [WIP] Feature output buffer middleware [In Review] Output buffer middleware Mar 8, 2017
@codeguy codeguy requested review from akrabat and geggleto March 8, 2017 15:22
Copy link
Member

@akrabat akrabat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Need tests for the new OutputBuffering middleware
  • Can you add a CHANGELOG entry please?

Update - I can't read!

@@ -296,36 +276,11 @@ public function testInvokeWhenEchoingOutput()
public function testInvokeWhenReturningAString()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test should return a string and expect an Exception to happen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akrabat I think the name of the test should change. This test should be renamed.

testEchoingOutputIsEmpty

testReturningStringThrowsException ?

// TODO: Do we keep output buffering?
if (is_callable([$route, 'setOutputBuffering'])) {
$route->setOutputBuffering($this->getSetting('outputBuffering', 'append'));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly think that we can remove it.

Dump and Die will still work, which is I feel what a majority of people do anyway.

@@ -296,36 +276,11 @@ public function testInvokeWhenEchoingOutput()
public function testInvokeWhenReturningAString()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akrabat I think the name of the test should change. This test should be renamed.

testEchoingOutputIsEmpty

testReturningStringThrowsException ?

ob_start();
$newResponse = $next($req, $res);
$output = ob_get_clean();
} catch (\Throwable $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP 5 vs PHP 7 #2175

@akrabat akrabat merged commit a10b833 into slimphp:4.x Jun 27, 2017
@akrabat akrabat changed the title [In Review] Output buffer middleware Output buffer middleware Oct 2, 2018
@l0gicgate l0gicgate mentioned this pull request Apr 25, 2019
@l0gicgate l0gicgate mentioned this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants