fix: corrects JSON encoding for various data types

[Justintime50]

Summary

This is an attempt at fixing JSON encoding for various data types. I tried to keep this as least destructive as possible (we could/should be using the JsonSerializable interface; however, I didn't want to introduce another larger rewrite if we could for now correct this with a simple pass.

This is accomplished now by:

1. Not using the array_filter which had some interesting side effects
2. Making any element that we previously would set as an empty array if no data was present to null instead
3. Sending all data as strings (as this was the previous behavior and what almost every field of our API expects)

Tests

• null tests
• empty string tests
• string tests
• array tests (empty and filled)
• float tests
• integer tests
• boolean tests

I added a few tests to check encoding and updated others. Looking at the output of cassettes, I'm much happier with this approach than the previous as it's behaving much closer to what I'd expect.

Docs about type comparisons: https://www.php.net/manual/en/types.comparisons.php

Closes #117

[fiendish]

Not using the array_filter which had some interesting side effects

Can you clarify what side effects?

Making any element that we previously would set as an empty array if no data was present to null instead

easypost-php/lib/EasyPost/Requestor.php

Lines 58 to 60 in a7d50bb

	if (!$data) {
	$data = array();
	}

still erroneously converts False into an array. I strongly suspect that it wants to be checking is_null instead of !.

Sending all integers as strings

I'm not clear on the purpose of checking for integer vs sending everything as strings, which was the previous inherent behavior.

[fiendish]

We are sending null instead of just dropping keys/objects if the user explicitly passes null. This is probably an acceptable behavior; however, we may want to simply drop them altogether?

Because you've removed the array filter? I think it wouldn't hurt to throw a null value check inside the is_array foreach before recursing.

Like, if we're only looking for strval conversion here and not yet applying more explicit field type checks, maybe ...

@@ -55,11 +55,9 @@
      */
     private static function _encodeObjects($data)
     {
-        if (!$data) {
-            $data = array();
-        }
-
-        if ($data instanceof EasypostResource) {
+        if (is_null($data)) {
+            return array();  // if the root is null, return an empty set of params at least
+        } elseif ($data instanceof EasypostResource) {
             return array("id" => self::utf8($data->id));
         } elseif ($data === true) {
             return 'true';
@@ -68,12 +66,14 @@
         } elseif (is_array($data)) {
             $resource = array();
             foreach ($data as $k => $v) {
-                $resource[$k] = self::_encodeObjects($v);
-            }
-
-            return array_filter($resource);
+                if (!is_null($v)) {
+                    $resource[$k] = self::_encodeObjects($v);
+                }
+            }
+
+            return $resource;
         } else {
-            return self::utf8($data);
+            return self::utf8(strval($data));
         }
     }

Though I didn't test it

[Justintime50]

Not using the array_filter which had some interesting side effects

Can you clarify what side effects?

Making any element that we previously would set as an empty array if no data was present to null instead

easypost-php/lib/EasyPost/Requestor.php

Lines 58 to 60 in a7d50bb

	if (!$data) {
	$data = array();
	}

still erroneously converts False into an array. I strongly suspect that it wants to be checking is_null instead of !.

Sending all integers as strings

I'm not clear on the purpose of checking for integer vs sending everything as strings, which was the previous inherent behavior.

I added the array_filter, it wasn't initially there and shouldn't have been. It's meant to be used with a function callback written by the user but we aren't even using it that way.

I think with your other suggested changes here we can easily fix the False issue.

[fiendish]

√ for passing tests, except for one weird thing

[fiendish]

It seems at this point like it may be cleaner to have all the early checks in _encodeObjects return null instead of array(), keep using some kind of filter on the array to remove them, and then have the original caller set array() if the end response there is null.

[fiendish]

What about

@@ -55,25 +55,26 @@
      */
     private static function _encodeObjects($data)
     {
-        if (!$data) {
-            $data = array();
-        }
-
-        if ($data instanceof EasypostResource) {
+        if (is_null($data) or ($data === "")) {
+            return null;
+        } elseif ($data instanceof EasypostResource) {
             return array("id" => self::utf8($data->id));
         } elseif ($data === true) {
             return 'true';
         } elseif ($data === false) {
             return 'false';
         } elseif (is_array($data)) {
+            if (empty($data)) {
+                return null;
+            }
+
             $resource = array();
             foreach ($data as $k => $v) {
                 $resource[$k] = self::_encodeObjects($v);
             }
-
-            return array_filter($resource);
+            return array_filter($resource, function($v) {return !is_null($v);});
         } else {
-            return self::utf8($data);
+            return self::utf8(strval($data));
         }
     }
 
@@ -152,6 +153,9 @@
 
         $absUrl = $this->apiUrl($url);
         $params = self::_encodeObjects($params);
+        if (is_null($params)) {
+            $params = array();
+        }
 
         $langVersion = phpversion();
         $uname = php_uname();

[fiendish]

👍

[klaxian]

Thank you very much for working on this!