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.

Sign up for GitHub

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

[2.0] Serializer interface #454

Closed
wants to merge 1 commit into from
Closed

[2.0] Serializer interface #454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 32 additions & 10 deletions lib/Raven/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ class Client
protected $error_types;

/**
* @var \Raven\Serializer $serializer
* @var SerializerInterface $serializer
*/
protected $serializer;
/**
* @var \Raven\Serializer $serializer
* @var SerializerInterface $serializer
*/
protected $reprSerializer;

Expand Down Expand Up @@ -341,8 +341,16 @@ public function init_with_options($options)
'name' => 'sentry-php',
'version' => self::VERSION,
]);
$this->serializer = new \Raven\Serializer($this->mb_detect_order);
$this->reprSerializer = new \Raven\ReprSerializer($this->mb_detect_order);
if (isset($options['serializer'])) {
$this->setSerializer($options['serializer']);
} else {
$this->serializer = new \Raven\Serializer($this->mb_detect_order);
}
if (isset($options['reprSerializer'])) {
$this->setReprSerializer($options['reprSerializer']);
} else {
$this->reprSerializer = new \Raven\ReprSerializer($this->mb_detect_order);
}
if (\Raven\Util::get($options, 'serialize_all_object', false)) {
$this->setAllObjectSerialize(true);
}
Expand Down Expand Up @@ -378,23 +386,33 @@ public function close_all_children_link()
/**
* Gets the representation serialier.
*
* @return ReprSerializer
* @return SerializerInterface
*/
public function getReprSerializer()
{
return $this->reprSerializer;
}

public function setReprSerializer(SerializerInterface $serializer)
{
$this->reprSerializer = $serializer;
}

/**
* Gets the serializer.
*
* @return Serializer
* @return SerializerInterface
*/
public function getSerializer()
{
return $this->serializer;
}

public function setSerializer(SerializerInterface $serializer)
{
$this->serializer = $serializer;
}

/**
* Installs any available automated hooks (such as error_reporting).
*/
Expand Down Expand Up @@ -1046,7 +1064,7 @@ public function sanitize(&$data)
$data['request'] = $this->serializer->serialize($data['request']);
}
if (!empty($data['user'])) {
$data['user'] = $this->serializer->serialize($data['user'], 3);
$data['user'] = $this->serializer->serialize($data['user'], ['max_depth' => 3]);
}
if (!empty($data['extra'])) {
$data['extra'] = $this->serializer->serialize($data['extra']);
Expand All @@ -1057,7 +1075,7 @@ public function sanitize(&$data)
}
}
if (!empty($data['contexts'])) {
$data['contexts'] = $this->serializer->serialize($data['contexts'], 5);
$data['contexts'] = $this->serializer->serialize($data['contexts'], ['max_depth' => 5]);
}
}

Expand Down Expand Up @@ -1602,7 +1620,11 @@ public function close_curl_resource()

public function setAllObjectSerialize($value)
{
$this->serializer->setAllObjectSerialize($value);
$this->reprSerializer->setAllObjectSerialize($value);
if (method_exists($this->serializer, 'setAllObjectSerialize')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Accordind to the comment left below about the serialize function definition, we could use the $context parameter to pass additional options to the serializer (in this case about serializing all objects) so that we don't need to rely on some methods not even defined in the interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not even defined in the interface
if (method_exists...

$this->serializer->setAllObjectSerialize($value);
}
if (method_exists($this->reprSerializer, 'setAllObjectSerialize')) {
$this->reprSerializer->setAllObjectSerialize($value);
}
}
}
35 changes: 31 additions & 4 deletions lib/Raven/Serializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
*
* @package raven
*/
class Serializer
class Serializer implements SerializerInterface
{
/*
* The default mb detect order
Expand Down Expand Up @@ -62,6 +62,33 @@ public function __construct($mb_detect_order = null)
}
}

/**
* Serialize an object (recursively) into something safe for data
* sanitization and encoding.
*
* @param mixed $value
* @param array $context
* @return string|bool|double|int|null|object|array
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation, and return value should be mixed (it's easier to read than having a so long list of scalar types)

Copy link
Contributor Author

@nokitakaze nokitakaze May 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I got this doc from this file itself. Do we want to change all serializing method to "return mixed" INCLUDING SerializerInterface::serialize?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would, because it's prettier imho

Copy link
Contributor Author

@nokitakaze nokitakaze May 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

"Pretiest" ways are always wrong in our art. This is the programming not a drawing Mona Lisa

Copy link
Contributor

Choose a reason for hiding this comment

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

mixed is used in places where a value can be of different types and it's appropriate here instead of a infinite list of possible return types (what if in the future we decide that we return class instances, etc?)

Copy link
Contributor Author

@nokitakaze nokitakaze May 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Accept — of course. But we are talking about param and return types, aren't we? Of course we should change param types to "mixed". I will do it. But what about return types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Return types should be only string, int, float, bool, array and null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait a minute

return value should be mixed

There is no other doc items which contain some long string with types. Only mixed value and array context.

Copy link
Contributor

@ste93cry ste93cry May 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was wrong in saying that the return value of the function should be that because the return types can only be scalars. I would use mixed anyway, because I find it prettier and easier to read, but feel free to do whathever you want on this. Doc is still missing for the $value and $context arguments and that is my main concern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc is still missing

I don't know what should I write. And I am afraid that my line should be corrected because english is not my native language. I know it isn't yours either.

*/
public function serialize($value, array $context = [])
{
$full_context = $this->get_full_context($context);
return $this->serializeInner($value, $full_context['max_depth'], 0);
}

/**
* @param array $context
* @return array
*/
protected function get_full_context(array $context)
{
if (!array_key_exists('max_depth', $context)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to always set this option? If he wants to serialize all objects he can't this way, but should be able to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean "max_depth"? I saved it for back compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

The max_depth option (correct me if I'm wrong) seems to be always set from the lines of code that call the serialize function, so backwards compatibility seems to be guaranteed. With these lines of code, the user won't be able to serialize everything regardless of depth because a max depth of 3 is always forced to be set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that call the serialize function

You are wrong. Serialize function could be called beyond \Raven\Client. So we must make sure $context['max_depth'] is set

Copy link
Contributor

Choose a reason for hiding this comment

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

Serialize function could be called beyond \Raven\Client

Is this referring to the fact that Client::serialize is public? Imho, it shouldn't because we're not a library to serialize things, so we don't provide a serializer that can be used for something else than serializing values being sent to a Sentry server. There is no need for this method to be public

Copy link
Contributor

@ste93cry ste93cry May 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You forgot that Sentry is a library for many real users

Trust me when I say that I always think about it, and rest assured that any decision I take is to give a clear public API to the end-users, and to me exposing something used internally is a bad practice. As this discussion is going too much further in time: @dcramer can you please say what's your opinion?

Copy link
Contributor Author

@nokitakaze nokitakaze May 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I just checked. There is no \Raven\Client::serialize. Only "sanitize".

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I meant to say that Client::getSerializer and Client::getReprSerializer are public when they shouldn't, but it was done because at that time I had no other choice. The plan is to remove them anyway. By the way, Client::sanitize should too not be public. Having all these methods private/protected means that there would be no way to access the serializer outside of the client (without inheriting the class, at least) and this means that there is no need the check the max_depth option because no BC would exists (as we control where the serializer is used). Does this makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tomorrow (2017/05/09) I will do pull request which will make all methods and properties protected and will break all compatibility we have at this moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make all methods and properties protected

Okay, It's already done: #457

$context['max_depth'] = 3;
}

return $context;
}

/**
* Serialize an object (recursively) into something safe for data
* sanitization and encoding.
Expand All @@ -71,13 +98,13 @@ public function __construct($mb_detect_order = null)
* @param int $_depth
* @return string|bool|double|int|null|object|array
*/
public function serialize($value, $max_depth = 3, $_depth = 0)
protected function serializeInner($value, $max_depth, $_depth)
{
if ($_depth < $max_depth) {
if (is_array($value)) {
$new = [];
foreach ($value as $k => $v) {
$new[$this->serializeValue($k)] = $this->serialize($v, $max_depth, $_depth + 1);
$new[$this->serializeValue($k)] = $this->serializeInner($v, $max_depth, $_depth + 1);
}

return $new;
Expand Down Expand Up @@ -111,7 +138,7 @@ public function serializeObject($object, $max_depth = 3, $_depth = 0, $hashes =
if (is_object($value)) {
$new_value = $this->serializeObject($value, $max_depth, $_depth + 1, $hashes);
} else {
$new_value = $this->serialize($value, $max_depth, $_depth + 1);
$new_value = $this->serialize($value, ['max_depth' => $max_depth - $_depth - 1]);
}
$return[$key] = $new_value;
}
Expand Down
17 changes: 17 additions & 0 deletions lib/Raven/SerializerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php


namespace Raven;

interface SerializerInterface
{
/**
* Serialize an object (recursively) into something safe for data
* sanitization and encoding.
*
* @param mixed $value
* @param array $context
* @return string|bool|double|int|null|object|array
*/
public function serialize($value, array $context = []);
}
2 changes: 1 addition & 1 deletion lib/Raven/Stacktrace.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Stacktrace implements \JsonSerializable
protected $client;

/**
* @var Serializer The serializer
* @var SerializerInterface The serializer
*/
protected $serializer;

Expand Down
136 changes: 133 additions & 3 deletions tests/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@

namespace Raven\Tests;

use Raven\Client;
use Raven\Serializer;

function simple_function($a = null, $b = null, $c = null)
{
assert(0);
Expand Down Expand Up @@ -2389,4 +2386,137 @@ public function testSetAllObjectSerialize()
$this->assertFalse($o1->getAllObjectSerialize());
$this->assertFalse($o2->getAllObjectSerialize());
}

public function dataDefaultSerializer()
{
return [['init_with_objects' => false], ['init_with_objects' => true],];
}

/**
* @param boolean $init_with_objects
*
* @dataProvider dataDefaultSerializer
*/
public function testDefaultSerializer($init_with_objects)
{
if ($init_with_objects) {
$client = new \Raven\Client(
null, [
'serializer' => new \Raven\Serializer(),
'reprSerializer' => new \Raven\ReprSerializer(),
]
);
} else {
$client = new \Raven\Client;
}
$this->assertEquals([1, 2, 3], $client->getSerializer()->serialize([1, 2, 3]));
$this->assertEquals([1, 2, 3], $client->getReprSerializer()->serialize([1, 2, 3]));
$this->assertTrue($client->getSerializer()->serialize(true));
$this->assertEquals('true', $client->getReprSerializer()->serialize(true));
}

public function dataExceptionOnMalformedSerializer()
{
return [
[
'property' => 'serializer',
'after_creation' => false,
], [
'property' => 'serializer',
'after_creation' => true,
], [
'property' => 'reprSerializer',
'after_creation' => false,
], [
'property' => 'reprSerializer',
'after_creation' => true,
],
];
}

/**
* @param string $property
* @param boolean $after_creation
*
* @expectedException \PHPUnit_Framework_Exception
* @dataProvider dataExceptionOnMalformedSerializer
*/
public function testExceptionOnMalformedSerializer($property, $after_creation)
{
if (version_compare(PHP_VERSION, '7.0.0', '>=')) {
$this->markTestSkipped('PHP 7.0.0 contain different error');
}
if (!$after_creation) {
new \Raven\Client(null, [$property => $this,]);
} else {
$client = new \Raven\Client(null, []);
$method = 'set'.$property;
$client->$method($this);
}
}

/**
* @param string $property
* @param boolean $after_creation
*
* @expectedException \TypeError
* @dataProvider dataExceptionOnMalformedSerializer
*/
public function testExceptionOnMalformedSerializerPHP70($property, $after_creation)
{
if (version_compare(PHP_VERSION, '7.0.0', '<')) {
$this->markTestSkipped('PHP 5.6.0 contain different error');
}
if (!$after_creation) {
new \Raven\Client(null, [$property => $this,]);
} else {
$client = new \Raven\Client(null, []);
$method = 'set'.$property;
$client->$method($this);
}
}

public function dataOurSerializer()
{
return [['after_creation' => true,], ['after_creation' => false,],];
}

/**
* @param boolean $after_creation
*
* @dataProvider dataOurSerializer
*/
public function testOurSerializer($after_creation)
{
$serializer = $this->getMockBuilder('\\Raven\\SerializerInterface')
->setMethods(['serialize'])->getMock();
$serializer->expects($this->atLeastOnce())->method('serialize');

/**
* @var \Raven\SerializerInterface|\PHPUnit_Framework_MockObject_MockObject $serializer
*/
if ($after_creation) {
$client = new Dummy_Raven_Client();
$client->setSerializer($serializer);
$client->setReprSerializer($serializer);
} else {
$client = new Dummy_Raven_Client(
null, [
'serializer' => $serializer,
'reprSerializer' => $serializer,
]
);
}

foreach (['bar', $this, 'tobe', null, false] as &$value) {
$client->captureMessage(
'Test Message %s', ['foo'], [
'extra' => [
'foo' => $value,
],
]
);
$serializer->__phpunit_verify();
}
}
}
21 changes: 12 additions & 9 deletions tests/SerializerAbstractTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,19 +186,19 @@ public function testRecursionMaxDepth($serialize_all_objects)
}
$input = [];
$input[] = &$input;
$result = $serializer->serialize($input, 3);
$result = $serializer->serialize($input, ['max_depth' => 3]);
$this->assertEquals([[['Array of length 1']]], $result);

$result = $serializer->serialize([], 3);
$result = $serializer->serialize([], ['max_depth' => 3]);
$this->assertEquals([], $result);

$result = $serializer->serialize([[]], 3);
$result = $serializer->serialize([[]], ['max_depth' => 3]);
$this->assertEquals([[]], $result);

$result = $serializer->serialize([[[]]], 3);
$result = $serializer->serialize([[[]]], ['max_depth' => 3]);
$this->assertEquals([[[]]], $result);

$result = $serializer->serialize([[[[]]]], 3);
$result = $serializer->serialize([[[[]]]], ['max_depth' => 3]);
$this->assertEquals([[['Array of length 0']]], $result);
}

Expand Down Expand Up @@ -297,7 +297,7 @@ public function testRecursionInObjects($object, $result_serialize, $result_seria
$serializer = new $class_name();
$serializer->setAllObjectSerialize(true);

$result1 = $serializer->serialize($object, 3);
$result1 = $serializer->serialize($object, ['max_depth' => 3]);
$result2 = $serializer->serializeObject($object, 3);
$this->assertEquals($result_serialize, $result1);
$this->assertEquals($result_serialize_object, $result2);
Expand All @@ -310,14 +310,17 @@ public function testRecursionMaxDepthForObject()
$serializer = new $class_name();
$serializer->setAllObjectSerialize(true);

$result = $serializer->serialize((object)['key' => (object)['key' => 12345]], 3);
$result = $serializer->serialize((object)['key' => (object)['key' => 12345]], ['max_depth' => 3]);
$this->assertEquals(['key' => ['key' => 12345]], $result);

$result = $serializer->serialize((object)['key' => (object)['key' => (object)['key' => 12345]]], 3);
$result = $serializer->serialize(
(object)['key' => (object)['key' => (object)['key' => 12345]]],
['max_depth' => 3]
);
$this->assertEquals(['key' => ['key' => ['key' => 12345]]], $result);

$result = $serializer->serialize(
(object)['key' => (object)['key' => (object)['key' => (object)['key' => 12345]]]], 3
(object)['key' => (object)['key' => (object)['key' => (object)['key' => 12345]]]], ['max_depth' => 3]
);
$this->assertEquals(['key' => ['key' => ['key' => 'Object stdClass']]], $result);
}
Expand Down