[2.0] Serializer interface #454

nokitakaze · 2017-04-30T13:59:20Z

I meant that we should provide a sort of SerializerInterface interface and a basic implementation (that actually can even check if the object being passed implements some methods like toDebugContext, but this is up to us) and if a user wants to send a custom serialization of an object then he can just (yes, using dependency injection) set the serializer to something else and do whatever he wants.

ste93cry · 2017-05-02T17:35:06Z

tests/ClientTest.php

@@ -210,6 +207,24 @@ public function join($timeout = null)
    }
 }

+class Dummy_Serializer implements \Raven\SerializerInterface


This seems bad because there is logic code in a test that doesn't reflect what the real implementation of the serializer does. You should use a mocked object instead

You should use a mocked object instead

I don't know how to create method with mock object library. That's why I never used it in my tests

doesn't reflect

Why?

The ClientTest::testOurSerializer method is actually testing the value returned from the serializer instance. I think that this is not appropriate, as the test class about the serializer should do these types of assertions. Instead, here we should check if the serializer is correctly set using the options/setters and if it's called. This is what mocks are for, they verify that something happens. PHPUnit doc is great and has a lot of examples on how to do it

we should check if the serializer is correctly set

Yes. This is exactly what I'm testing. Look at this https://github.com/getsentry/sentry-php/pull/454/files#diff-668268a1040801a44451100bf9062b17R2510
I set some serializer and then I check if it has been set

I know the scope of that test is to check that the serializer is correctly set, but there is no need to check the value returned after serializing a message to do it. You should create a mock, pass it in place of the current Dummy_Serializer object and assert that its serialize method is called when capturing an exception/message. Nothing more, nothing else. No checks on how the sent message is serialized itself

and assert that its serialize method is called

Okay. I will change it tomorrow

ste93cry · 2017-05-02T17:35:45Z

lib/Raven/SerializerInterface.php

+     * @param int   $_depth
+     * @return string|bool|double|int|null|object|array
+     */
+    public function serialize($value, $max_depth = 3, $_depth = 0);


What about changing the signature of this method to serialize($value, array $context = [])? This way, it's more standardized according to other serializer libraries and mostly important it allows us to pass additional options in the future without breaking compatibility by adding a new method to the interface

How should I change signature of existing method without breaking back compatibility? @dcramer will grumble

And before I break BC I want to know the maintainer's opinion

Fortunately, as we're working on 2.x branch we can break compatibility with older versions, so there shouldn't be any problem in doing it. Plus, I think it was a really big mistake to have all properties/methods public in the 1.x version. We should expose only things that actually serves a purpose and I don't think that a public serialize method is so useful since the scope of this lbrary is not to be a serializer. So, here what I would do: make the Serializer::serializeObject protected (there is no reason to let it public), change the signature of the Serializer::serialize method according to the previously suggested definition (yes, breaking compatibility) and rework the code calling it to use the $context argument to pass the old arguments like $max_depth as key/values of the array. Plus, the $depth argument is something related to internal behaviour and should not be exposed anyway in a public method. One final though: should we provide a basic implementation of a serializer or should we just delegate it to a 3rd party library? I mean, there are a lot of cases to handle (circular references, how to cast objects/closures/different types) and there are already existing projects that do it well. Why should we reinvent the wheel?

should we provide a basic implementation of a serializer

We did it already. \Raven\Serializer is it. Anyone can extend it

make the Serializer::serializeObject protected
change the signature of

I will do it tomorrow.

it was a really big mistake to have all properties/methods public

Okay. Which of us will make a pull request that will make all methods protected?

Okay. Which of us will make a pull request that will make all methods protected?

This is off-topic, but I don't think there is a need now to open a PR to change all methods from public to protected because we will probably do it with the future PRs regarding other changes. Mine was just a statement,related to public methods that were strictly connected with the serializer (Client::serialize for example): only those should be changed in this PR

ste93cry · 2017-05-02T17:37:09Z

lib/Raven/Client.php

@@ -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')) {


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

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

ste93cry · 2017-05-06T16:32:57Z

tests/ClientTest.php

-            $events = $client->getSentEvents();
-            $event = array_pop($events);
-            $this->assertEquals(Dummy_Serializer::$default_value, $event['extra']['foo']);
+            $serializer->__phpunit_verify();


This is wrong. There is no need to call an internal PHPUnit function to assert that a given method has been called. You should mock the methods and then just call the code as you would in the real use-case and PHPUnit will do the rest for you.

I don't know how to check asserting after each call. Assert must be done in every iteration of foreach block

As we're not testing anymore the return value of the serializer, the whole foreach block isn't needed. Just capture a single message and check that the serialize method of the mocked object is called

We must be sure serialize is invoked on every accessible type. I create foreach block for this and only for this purpose.

No we don't. we just have to test that the Serializer::serialize method is called when appropriate, e.g. when sending a message to the Sentry server. There is no reason to test that (this is just an example) a boolean is passed to the method

ste93cry · 2017-05-06T16:35:11Z

lib/Raven/SerializerInterface.php

@@ -10,9 +10,8 @@
     * sanitization and encoding.
     *
     * @param mixed $value
-     * @param int   $max_depth
-     * @param int   $_depth
+     * @param array $context


Missing doc for the argument. Plus, I would just put mixed as return value instead of a long list of scalar types

Which argument I missed?

$value and $mixed don't have a description

ste93cry · 2017-05-06T16:38:30Z

lib/Raven/Serializer.php

     * @param int   $max_depth
     * @param int   $_depth
     * @return string|bool|double|int|null|object|array
     */
-    public function serialize($value, $max_depth = 3, $_depth = 0)
+    protected function serialize_inner($value, $max_depth, $_depth)


This comment applies to any code: properties, variables and methods should all use camel case, so for example $max_depth should be renamed to $maxDepth

ste93cry · 2017-05-06T16:39:56Z

lib/Raven/Serializer.php

@@ -67,17 +67,44 @@ public function __construct($mb_detect_order = null)
     * sanitization and encoding.
     *
     * @param mixed $value
+     * @param array $context
+     * @return string|bool|double|int|null|object|array


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

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

I would, because it's prettier imho

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

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?)

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?

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

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.

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

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.

ste93cry · 2017-05-06T16:41:38Z

lib/Raven/Serializer.php

+     */
+    protected function get_full_context(array $context)
+    {
+        if (!array_key_exists('max_depth', $context)) {


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

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

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

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

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

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?

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

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?

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

make all methods and properties protected

Okay, It's already done: #457

ste93cry · 2018-11-21T20:13:21Z

I'm going to close this as it's really old and it has been superseded by multiple PRs attempting to solve the same issue, last of all #701

nokitakaze requested a review from ste93cry April 30, 2017 13:59

nokitakaze force-pushed the feature/SerializerInterface branch from 9f837f6 to d56a1b6 Compare April 30, 2017 14:07

ste93cry suggested changes May 2, 2017

View reviewed changes

ste93cry suggested changes May 6, 2017

View reviewed changes

+ SerializerInterface

d053f83

nokitakaze force-pushed the feature/SerializerInterface branch from e736e40 to d053f83 Compare May 6, 2017 17:22

This was referenced May 9, 2017

toSentry method for more meaningful data #443

Closed

Implemented option for serializer max_depth #427

Closed

Jean85 added this to the Release 2.0 milestone May 23, 2017

nokitakaze mentioned this pull request Oct 23, 2017

Test PHP 5.3 in separate environment #503

Merged

stayallive changed the title ~~Serializer interface~~ [2.0] Serializer interface Oct 29, 2017

nokitakaze mentioned this pull request Jan 2, 2018

[2.0] Ideas and proposals #424

Closed

Jean85 mentioned this pull request Jan 20, 2018

[WIP] [2.0] Serializer interface refactor #534

Closed

Jean85 mentioned this pull request May 4, 2018

[2.0] Road to the 2.0 release #596

Closed

15 tasks

ste93cry closed this Nov 21, 2018

[2.0] Serializer interface #454

[2.0] Serializer interface #454

Conversation

nokitakaze commented Apr 30, 2017

Choose a reason for hiding this comment

nokitakaze May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nokitakaze May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nokitakaze May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nokitakaze May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nokitakaze May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ste93cry May 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ste93cry May 7, 2017 • edited Loading

Choose a reason for hiding this comment

nokitakaze May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ste93cry commented Nov 21, 2018

nokitakaze May 2, 2017 •

edited

Loading

nokitakaze May 2, 2017 •

edited

Loading

nokitakaze May 6, 2017 •

edited

Loading

nokitakaze May 6, 2017 •

edited

Loading

nokitakaze May 6, 2017 •

edited

Loading

ste93cry May 7, 2017 •

edited

Loading

ste93cry May 7, 2017 •

edited

Loading

nokitakaze May 8, 2017 •

edited

Loading