Compatibility with PHP 7.3

sodabrew commented 6 years ago

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]

sodabrew commented 5 years ago

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

sodabrew commented 5 years ago

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 commented 5 years ago

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

sodabrew commented 5 years ago

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 commented 5 years ago

@sodabrew are you sure that msgpack unsupported?

vedadkajtaz commented 5 years ago

@andypost it does not compile with 7.3

inverse commented 5 years ago

Any update on this PR?

rlerdorf commented 5 years ago

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

sodabrew commented 5 years ago

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

msgpack / msgpack-php

Compatibility with PHP 7.3 #124