Skip to content

Add Content-type plugin to set content-type header automatically #85

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

Merged
merged 9 commits into from
Aug 5, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions spec/Plugin/ContentTypePluginSpec.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php

namespace spec\Http\Client\Common\Plugin;

use PhpSpec\Exception\Example\SkippingException;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\StreamInterface;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class ContentTypePluginSpec extends ObjectBehavior
{
function it_is_initializable()
{
$this->shouldHaveType('Http\Client\Common\Plugin\ContentTypePlugin');
}

function it_is_a_plugin()
{
$this->shouldImplement('Http\Client\Common\Plugin');
}

function it_adds_json_content_type_header(RequestInterface $request)
{
$request->hasHeader('Content-Type')->shouldBeCalled()->willReturn(false);
$request->getBody()->shouldBeCalled()->willReturn(json_encode(['foo' => 'bar']));
$request->withHeader('Content-Type', 'application/json')->shouldBeCalled()->willReturn($request);

$this->handleRequest($request, function () {}, function () {});
}

function it_adds_xml_content_type_header(RequestInterface $request)
{
$request->hasHeader('Content-Type')->shouldBeCalled()->willReturn(false);
$request->getBody()->shouldBeCalled()->willReturn('<foo>bar</foo>');
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a real stream implementation instead of a string ?

I think this test case will fail with a stream implementation as there is no rewind

$request->withHeader('Content-Type', 'application/xml')->shouldBeCalled()->willReturn($request);

$this->handleRequest($request, function () {}, function () {});
}

function it_does_not_set_content_type_header(RequestInterface $request)
{
$request->hasHeader('Content-Type')->shouldBeCalled()->willReturn(false);
$request->getBody()->shouldBeCalled()->willReturn('foo');
$request->withHeader('Content-Type', null)->shouldNotBeCalled();

$this->handleRequest($request, function () {}, function () {});
}

function it_does_not_set_content_type_header_if_already_one(RequestInterface $request)
{
$request->hasHeader('Content-Type')->shouldBeCalled()->willReturn(true);
$request->getBody()->shouldNotBeCalled()->willReturn('foo');
$request->withHeader('Content-Type', null)->shouldNotBeCalled();

$this->handleRequest($request, function () {}, function () {});
}

}
62 changes: 62 additions & 0 deletions src/Plugin/ContentTypePlugin.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

namespace Http\Client\Common\Plugin;

use Http\Client\Common\Plugin;
use Psr\Http\Message\RequestInterface;

/**
* Allow to set the correct content type header on the request automatically only if it is not set .
*
* @author Karim Pinchon <[email protected]>
*/
final class ContentTypePlugin implements Plugin
Copy link
Member

Choose a reason for hiding this comment

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

May be nice to add some options to this plugin to avoid detection on too large stream, or stream with a null size

{
/**
* {@inheritdoc}
*/
public function handleRequest(RequestInterface $request, callable $next, callable $first)
{
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid content type detection if stream is not rewindable

if (!$request->hasHeader('Content-Type')) {
Copy link
Member

Choose a reason for hiding this comment

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

We should early return here if the body size of the stream is equal to 0

$stream = $request->getBody();

if ($this->isJson($stream)) {
$request = $request->withHeader('Content-Type', 'application/json');

return $next($request);
}

if ($this->isXml($stream)) {
$request = $request->withHeader('Content-Type', 'application/xml');

return $next($request);
}
Copy link
Member

Choose a reason for hiding this comment

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

Some content type that can be added to check (but not necessarly in this PR) :

  • Image types by using a library image (should be optional IMO)
  • Binary type by checking if there is a null character (\0) in the stream (that's how git does to detect binary vs text)
  • Text type (inverse of the binary type, imo the default content type we want to add to the stream)

}

return $next($request);
}

/**
* @param $stream
*
* @return bool
*/
private function isJson($stream)
{
json_decode($stream);

Copy link
Member

Choose a reason for hiding this comment

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

We should rewind the stream here

return json_last_error() == JSON_ERROR_NONE;
}

/**
* @param $stream
*
* @return \SimpleXMLElement|false
*/
private function isXml($stream)
{
libxml_use_internal_errors(true);
Copy link
Member

Choose a reason for hiding this comment

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

Should we reset to the original value?


Copy link
Member

Choose a reason for hiding this comment

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

We should rewind the stream here

return simplexml_load_string($stream);
}
}