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

Jump to bottom

Several improvements / Fixes #36

Closed

jevadebe wants to merge 2 commits into zumba:master from jevadebe:master

jevadebe commented Sep 21, 2018

Hi,

I made several improvements to the library.

Fixed an issue where array`s by reference were duplicated.
Fixed an issue where static properties where being restored.
Created new magic method invoked after creating object, but before restoring data
Updated readme

Yours, Jeroen

jvdberg added 2 commits

September 21, 2018 19:14


          Add magic method __wakeupprerestore & Support references for array`s …

22ae2a8

…& Ignore static properties


          Update readme

57d0544

jrbasso requested changes

View reviewed changes

Member

jrbasso left a comment

@jevadebe Thanks for the suggestions, but I guess it needs some updates that I pointed inline.

Also, can you make it conform with PSR guidelines?

src/JsonSerializer/JsonSerializer.php

                    * @return mixed
                    * @throws JsonSerializerException
                    */
-                  protected function serializeData($value)
+                  protected function serializeData(&$value)

Member

jrbasso Sep 21, 2018

Why to pass the by reference?

src/JsonSerializer/JsonSerializer.php

@@ @@ -259,18 +261,31 @@ protected function serializeData($value) @@
                           throw new JsonSerializerException('Resource is not supported in JsonSerializer');
                       }
                       if (is_array($value)) {
-                          return array_map(array($this, __FUNCTION__), $value);
+                          $id = uniqid();
+                          $value[$id] = true;

Member

jrbasso Sep 21, 2018

This would change the passed content since it came as reference. Sounds weird to change it.

src/JsonSerializer/JsonSerializer.php

                               static::CLOSURE_IDENTIFIER_KEY => true,
                               'value' => $this->closureSerializer->serialize($value)
                           );
+                          return $output;

Member

jrbasso Sep 28, 2018

Any reason to break down the output? Or is it something left from debugging?

src/JsonSerializer/JsonSerializer.php

                       }
-                      return $this->serializeObject($value);
+                      $obj = $this->serializeObject($value);
+                      return $obj;

Member

jrbasso Sep 28, 2018

Same about breaking down the return.

src/JsonSerializer/JsonSerializer.php

@@ @@ -320,7 +338,8 @@ protected function getObjectProperties($ref, $value) @@
                       $props = array();
                       foreach ($ref->getProperties() as $prop) {
-                          $props[] = $prop->getName();
+                          if(!$prop->isStatic())

Member

jrbasso Sep 28, 2018

I guess storing or not storing static on the output should be an option on the [de]serialization.

src/JsonSerializer/JsonSerializer.php

@@ @@ -434,6 +472,13 @@ protected function unserializeObject($value) @@
                           return $this->objectMapping[$index];
                       }
+                      if ($className[0] === '[') {
+                          $index = substr($className, 1, strlen($className) - 2);
+                          // dd($index);

Member

jrbasso Sep 28, 2018

Remove another debug code.

src/JsonSerializer/JsonSerializer.php

@@ @@ -434,6 +472,13 @@ protected function unserializeObject($value) @@
                           return $this->objectMapping[$index];
                       }
+                      if ($className[0] === '[') {

Member

jrbasso Sep 28, 2018

Maybe you should check for the pattern completely to avoid false positives and bad deserialization.

src/JsonSerializer/JsonSerializer.php

@@ @@ -463,22 +508,54 @@ protected function unserializeObject($value) @@
                           return $obj;
                       }
+                      if (method_exists($obj, '__wakeupprerestore')) {

Member

jrbasso Sep 28, 2018

Which method is that? Why to call it? I don't think it's standard.

tests/JsonSerializerTest.php

@@ @@ -214,6 +214,7 @@ public function testUnserializeObjects() @@
                   {
                       $serialized = '{"@type":"stdClass"}';
                       $obj = $this->serializer->unserialize($serialized);
+                      // var_dump($obj);

Member

jrbasso Sep 28, 2018

Please remove debug code.

tests/JsonSerializerTest.php

+                      $unserialized->arr[] = "This is arr";
+                      $unserialized->arr2[] = "This is arr2";
+                      $this->assertTrue($unserialized->arr_copy[2] === "This is arr");

Member

jrbasso Sep 28, 2018

You should be using assertSame instead using the ===.

young-steveo commented Oct 29, 2018

Closing due to inactivity. @jevadebe let us know if you update per @jrbasso's review.

young-steveo closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment