Compatibility with PHP 7.3

[sodabrew]

No description provided.

[sodabrew]

Woo! It compiles! Tests are now failing in exactly the expected manner due to changes in PHP master:

========DIFF========
002+ 82a16182a162a163a164a165a16683c001a16182a162a163a164a165a166c0
002- 82a16182a162a163a164a165a16683c001a16182a162a163a164a165a16682c0020003
012+   array(2) {
012-   &array(2) {
021+     NULL
021-     *RECURSION*
041+   array(3) {
041-   &array(3) {
049+       NULL
049-       *RECURSION*
055+   array(3) {
055-   &array(3) {
063+       NULL
063-       *RECURSION*
========DONE========
FAIL Cyclic array test [tests/026.phpt] 

[remicollet]

I think the test failures may be related to this part.
Z_IS_RECURSIVE is mostly equiv. to ZEND_HASH_GET_APPLY_COUNT > 0 (not > 1)

[sodabrew]

Thanks for the review! What do you recommend to bridge this gap? Is there a different check that will say if a single count is present?

[remicollet]

Sadly, I have no idea, probably @laruence will know

[sodabrew]

Interesting - the in-tree JSON extension is using GC_IS_RECURSIVE instead of Z_IS_RECURSIVE: https://github.com/php/php-src/blob/master/ext/json/json_encoder.c#L143

[remicollet]

I was able to fix the yaml ext which have the same issue
php/pecl-file_formats-yaml#33

But this require a bit more change... need to find the proper fix.

[sodabrew]

I think this is working now for PHP 7.3, but it still fails for PHP master. I'm waiting for travis-ci/travis-ci#9717 to get a PHP 7.3 image on Travis CI.

[sodabrew]

I think I understand the crux of the problem. This module has always behaved incorrectly and the tests were configured to ensure incorrect behavior.

Given this recursive scenario:

$a = array(null);
$b = array(&$a);
$a[0] = &$b;

var_dump($a);

In all versions of PHP 7.x the output of var_dump is:

array(1) {
  [0]=>
  &array(1) {
    [0]=>
    &array(1) {
      [0]=>
      *RECURSION*
    }
  }
}

The unit tests pack and unpack the variable to see how it goes through msgpack:

$serialized = msgpack_pack($a);
$unserialized = msgpack_unpack($serialized);
var_dump($unserialized);

The unit tests then expect to see the following output. Note the expectation of iterating the recursion exactly once, whereas PHP itself does not iterate the recursion at all as seen above:

array(1) {
  [0]=>
  array(1) {
    [0]=>
    array(1) {
      [0]=>
      array(1) {
        [0]=>
        array(1) {
          [0]=>
          NULL
        }
      }
    }
  }
}

Therefore I believe the correct fix is to change the unit tests to match the PHP 7.3 required behavior (i.e. no longer being able to check that we've been through the recursion just once, but rather only being able to check before the zero-th pass) and to change the code to operate the same way for PHP 7.0-7.2.

[sodabrew]

And there we have it, tests! Anybody around to code review now?

[sodabrew]

Hellooooo PHP 7.3.0 is released already. I'm a bit blocked on updating the memcached pecl module, unless I release-note that msgpack is no longer supported.

[andypost]

@sodabrew are you sure that msgpack unsupported?

[vedadkajtaz]

@andypost it does not compile with 7.3

[inverse]

Any update on this PR?

[rlerdorf]

I have tested this PR and the changes look good. Could we get this merged, please?

[sodabrew]

@laruence perhaps you’ve seen this PR since before you did similar work this week? Did I waste my time here?