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

Replace addContentLengthHeader with ContentLength Middleware #2254

Merged
merged 5 commits into from
Aug 17, 2017
Merged

Replace addContentLengthHeader with ContentLength Middleware #2254

merged 5 commits into from
Aug 17, 2017

Conversation

akrabat
Copy link
Member

@akrabat akrabat commented Jun 29, 2017

This feature isn't needed in core and is better as optional middleware.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 93.379% when pulling 3520c44 on akrabat:content-length-middleware into 4556842 on slimphp:4.x.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 93.379% when pulling b86ef64 on akrabat:content-length-middleware into 4556842 on slimphp:4.x.

@damianopetrungaro
Copy link

@akrabat
As discussed on Slack for the AppTest:

public function testAddSettings()
{
    $app = new App();
    $app->addSettings(['foo' => 'bar']);
    $this->assertAttributeContains('bar', 'settings', $app);
}

public function testAddSetting()
{
    $app = new App();
    $app->addSetting('foo', 'bar');
    $this->assertAttributeContains('bar', 'settings', $app);
}

with this it works 😄

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 93.371% when pulling 54db772 on akrabat:content-length-middleware into 4556842 on slimphp:4.x.

@damianopetrungaro
Copy link

Or even better:

public function testAddSettings()
{
    $app = new App();
    $reflection = new ReflectionProperty(App::class, 'settings');
    $reflection->setAccessible(true);
    $reflection->setValue($app, []);
    $app->addSettings(['foo' => 'bar']);
    $this->assertSame(['foo' => 'bar'], $reflection->getValue($app));
}

public function testAddSetting()
{
    $app = new App();
    $reflection = new ReflectionProperty(App::class, 'settings');
    $reflection->setAccessible(true);
    $reflection->setValue($app, []);
    $app->addSetting('foo', 'bar');
    $this->assertSame(['foo' => 'bar'], $reflection->getValue($app));
}

@akrabat
Copy link
Member Author

akrabat commented Aug 13, 2017

Ping @geggleto

// Add Content-Length header if not already added
$size = $response->getBody()->getSize();
if ($size !== null && !$response->hasHeader('Content-Length')) {
$response = $response->withHeader('Content-Length', (string) $size);
Copy link
Member

Choose a reason for hiding this comment

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

.... why are we casting it to a string? ... my gut says that is a really bad idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

withHeader() is defined as taking a string as the second parameter.


$newResponse = $mw($request, $response, $next);

$this->assertEquals(4, $newResponse->getHeaderLine('Content-Length'));
Copy link
Member

Choose a reason for hiding this comment

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

This test should also assert that the body's content is actually 'Body'

@akrabat what about multi-byte character sets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? The code under test doesn't touch the body and ensuring the body is the correct text is covered by tests on Response and App.

Copy link
Member Author

@akrabat akrabat Aug 15, 2017

Choose a reason for hiding this comment

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

Size of multi-byte character sets is an issue for Slim\Http\Stream() and should be tested there I think?

i.e. I'm testing that the correct header is added as that's what this Middleware does.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's an argument that the test should be:

$this->assertEquals($response->getBody()->getSize(), $newResponse->getHeaderLine('Content-Length'));

or even:

$this->assertTrue($newResponse->hasHeader('Content-Length'));

@akrabat
Copy link
Member Author

akrabat commented Aug 15, 2017

Renamed to ContentLengthMiddleware as per #2288 (comment)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 93.008% when pulling dce758e on akrabat:content-length-middleware into 4556842 on slimphp:4.x.

UPGRADING.md Outdated
@@ -1,9 +1,11 @@
# How to upgrade

* [2254] - You need to add the `Middleware\ContentLength` middleware if you want Slim to add this automatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

ContentLengthMiddleware?
Maybe the OutputBuffering should also be renamed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks.

Yes, 'OutputBuffering' will need renaming too, but that's a separate PR if you fancy doing it?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 93.008% when pulling 6a716f2 on akrabat:content-length-middleware into 4556842 on slimphp:4.x.

@akrabat
Copy link
Member Author

akrabat commented Aug 17, 2017

Rebased to fix conflicts

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 93.008% when pulling 00c31cf on akrabat:content-length-middleware into a60a712 on slimphp:4.x.

@akrabat akrabat merged commit 00c31cf into slimphp:4.x Aug 17, 2017
akrabat added a commit that referenced this pull request Aug 17, 2017
@akrabat akrabat added this to the 4.0 milestone Aug 24, 2018
@akrabat akrabat deleted the content-length-middleware branch April 18, 2019 06:10
@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants