martin-schilling / php-velocypack

PHP 7.1+ extension wrapping the arangodb/velocypack library
6 stars 0 forks source link

PHP 7.3 converts indexed array to object #12

Closed sandrokeil closed 5 years ago

sandrokeil commented 5 years ago

See Travis

1) ArangoDbTest\StatementTest::it_supports_json_arbitrary_data
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'["test"]'
+'{"0":"test"}'
/home/travis/build/sandrokeil/arangodb-php-client/test/StatementTest.php:176
martin-schilling commented 5 years ago

I was able to reproduce it. Not quite sure why but array_merge seems to be the problem:

<?php

$vpack1 = \Velocypack\Vpack::fromArray(array_merge([], ['test']));
$vpack2 = \Velocypack\Vpack::fromArray(['test']);

var_dump(phpversion());

var_dump(['test']);
var_dump(array_merge([], ['test']));
var_dump($vpack1->toJson());
var_dump($vpack2->toJson());

produces

string(5) "7.3.0"
array(1) {
  [0]=>
  string(4) "test"
}
array(1) {
  [0]=>
  string(4) "test"
}
string(12) "{"0":"test"}"
string(8) "["test"]"
martin-schilling commented 5 years ago

I found out a few more interesting things about this. First of all if you use array_merge with the arguments switched the problem is gone. Then I tried json_encode and noticed that it works in all cases as one would expect. Therefore I have been looking at its implementation and found out that it not only uses the HT_IS_PACKED(myht) && HT_IS_WITHOUT_HOLES(myht) flags that are used here as well, but also loops through the entire array for checking if it is in fact a numeric one. https://github.com/php/php-src/blob/a5fa51afbbd87bedeb1c5fd7c9a6cf3c971ab14c/ext/json/json_encoder.c#L49-L63

I am going to implement it the exact same way here as well to prevent issues with this in the future.

martin-schilling commented 5 years ago

So this should solve the problem: https://github.com/martin-schilling/php-velocypack/commit/c20c44e1690af119df224d5efa28ba2e06fec1fb

The extension now pretty much uses the same checks that json_encode uses to determine if an array is numeric or not, therefore this kind of error should not pop up in the future again.

@sandrokeil Please try if this solves the problem :smile:

sandrokeil commented 5 years ago

All green again. :+1: