zephir-lang / zephir

Zephir is a compiled high-level language aimed to ease the creation of C-extensions for PHP
https://zephir-lang.com
MIT License
3.3k stars 466 forks source link

a property is INCONSISTENT, when the property is ARRAY in a class with MAGIC method #438

Closed dogstarTest closed 3 years ago

dogstarTest commented 10 years ago

Hi Guys,

It seems that I find a very strange question. I check my code carefully and debug it, but I really found that if I have a class property with array type, I got NULL returned after I set it "correctly" with magic method. That is the POINT: why the property is inconsistent?! When i dump the property inside the class, it showed as expected. When I get it outside or use the property, however, some index was NULL! What's worse, the property changed unexpected!

To show the question clearly, I will attach my original code and the PHPUnit test, of course, and the phpunit result, my ENV as well.

Maybe it is a little long, to save your time, you can run the code and test in your local envirenment. THANKS.

ENV:

System: Ubuntu 11.10 PHP 5.3.10-1ubuntu3.12 with Suhosin-Patch (cli) (built: Jun 20 2014 00:36:12) Zephir version 0.4.2a PHPUnit 4.0.17 by Sebastian Bergmann.

ORIGINAL CODE:

//MagicDi.zep
namespace Dogstar;

class MagicDi
{
    protected _data = [];

    public function __construct()
    {
    }

    public function set(var key, var value)
    {
        echo "#set# key = " . key . "; value = " . value ."\n";

        this->_checkKey(key);

        let this->_data[key] = value;

        echo "#data after set# ";
        var kk, vv;
        for kk, vv in this->_data {
            echo kk . " -> " . value ."\t";
        }
        echo "\n";
    }

    public function get(key, defaultValue = null)
    {
        echo "#get# key = " . key . "\n";

        this->_checkKey(key);

        var value;
        if !(fetch value, this->_data[key]) {
            return defaultValue;
        }

        let value = this->_data[key];

        echo "#fetch after get# key = " . key . "; value = " . value . "(" . gettype(value) . ")" . "\n";

        return value;
    }

    public function getData()
    {
        return this->_data;
    }

    protected function _checkKey(var key)
    {
        if empty(key) || (!is_string(key) && !is_numeric(key)) {
            throw new \Exception("Unvalid key(" . gettype(key) . "), expect to string or numeric");
        }
    }

    public function __call(name, params)
    {
        var prefix;
        let prefix = substr(name, 0, 3);

        var key;
        let key = lcfirst(substr(name, 3, strlen(name)));

        var value = null;
        fetch value, params[0];

        echo "#__call# method = " . prefix . "; key = " . key . "; value = " . value . "\n ";

        if prefix == "get" {
            return this->get(key, value);
        }

        if prefix == "set" {
            this->set(key, value);
            return;
        }

        throw new \Exception("Call to undefined method Di::" . name . "()");
    }
}

PHPUNIT TEST:

//test_MagicDi.php
<?php
class MagicDi_Test extends PHPUnit_Framework_Testcase
{
    public $fdi;

    public function setUp()
    {
        $this->fdi = new Dogstar\MagicDi();
    }

    public function testSetAndGetMethod()
    {
        $di = $this->fdi;
        $this->assertNotNull($di);

        $di->set('name', 'phalcon');
        $rs = $di->get('name');
        $this->assertEquals('phalcon', $rs);
    }

    public function testMagicMethod()
    {
        $di = $this->fdi;

        $di->setName('phalcon');
        var_dump($di->getData());   //WHY shows: array('name' => NULL)

        $rs = $di->getName();
        $this->assertEquals('phalcon', $rs);    //FAILED!
    }

    public function testDataProperty()
    {
        $this->fdi->set('key1', 'value1');
        $this->fdi->set('key2', 'value2');
        $this->fdi->setKey3('value3');
        $this->fdi->set('key4', 'value4');
        $this->fdi->setKey5('value5');

        var_dump($this->fdi->getData());    //WHY key3 and key5 is NULL ?!
        $this->assertAttributeEquals($this->fdi->getData(), '_data', $this->fdi);
        $expectedData = array(
                'key1' => 'value1',
                'key2' => 'value2',
                'key3' => 'value3',
                'key4' => 'value4',
                'key5' => 'value5',
                );
        var_dump($this->fdi->getData());    //WHY key3 and key5 change ?!
        $this->assertEquals($expectedData, $this->fdi->getData()); //FAILED!
    }

    public function testMagicSetterAndGet()
    {
        $this->fdi->setVersion('1.0.0');
        $rs = $this->fdi->get('version');
        $this->assertNotNull($rs);
        $this->assertEquals('1.0.0', $rs);  //WHY version change to version ?!
    }
}

PHPUNIT RESULT:

Time: 13 ms, Memory: 3.50Mb

There were 3 failures:

1) MagicDi_Test::testMagicMethod
Failed asserting that null matches expected 'phalcon'.

/home/dogstar/projects/php/test/test_dogstar/test_MagicDi.php:29

2) MagicDi_Test::testDataProperty
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'key1' => 'value1'
     'key2' => 'value2'
-    'key3' => 'value3'
+    'key3' => 'value1'
     'key4' => 'value4'
-    'key5' => 'value5'
+    'key5' => 'value1'
 )

/home/dogstar/projects/php/test/test_dogstar/test_MagicDi.php:50

3) MagicDi_Test::testMagicSetterAndGet
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1.0.0'
+'version'

/home/dogstar/projects/php/test/test_dogstar/test_MagicDi.php:58

FAILURES!
Tests: 4, Assertions: 7, Failures: 3.

MORE DETAIL WHEN PHPUNIT DEBUG

Starting test 'MagicDi_Test::testSetAndGetMethod'. .#set# key = name; value = phalcon

data after set# name -> phalcon

get# key = name

fetch after get# key = name; value = phalcon(string)

Starting test 'MagicDi_Test::testMagicMethod'. F#__call# method = set; key = name; value = phalcon

set# key = name; value = phalcon

data after set# name -> phalcon

array(1) { ["name"]=> NULL }

__call# method = get; key = name; value =

get# key = name

fetch after get# key = name; value = (NULL)

Starting test 'MagicDi_Test::testDataProperty'. F#set# key = key1; value = value1

data after set# key1 -> value1

set# key = key2; value = value2

data after set# key1 -> value2 key2 -> value2

__call# method = set; key = key3; value = value3

set# key = key3; value = value3

data after set# key1 -> value3 key2 -> value3 key3 -> value3

set# key = key4; value = value4

data after set# key1 -> value4 key2 -> value4 key3 -> value4 key4 -> value4

__call# method = set; key = key5; value = value5

set# key = key5; value = value5

data after set# key1 -> value5 key2 -> value5 key3 -> value5 key4 -> value5 key5 -> value5

array(5) { ["key1"]=> string(6) "value1" ["key2"]=> string(6) "value2" ["key3"]=> NULL ["key4"]=> string(6) "value4" ["key5"]=> NULL } array(5) { ["key1"]=> string(6) "value1" ["key2"]=> string(6) "value2" ["key3"]=> string(6) "value1" ["key4"]=> string(6) "value4" ["key5"]=> string(6) "value1" }

THANKS! And I love Zephir!

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/3031802-a-property-is-inconsistent-when-the-property-is-array-in-a-class-with-magic-method?utm_campaign=plugin&utm_content=tracker%2F280146&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F280146&utm_medium=issues&utm_source=github).
nkt commented 10 years ago

Please split your not correctly working code. Also you write tests in wrong way. You should mock object and check does set method call, or not. If phpunit could not mock built-in classes, you can compare objects for example like this:

public function testMagicCall()
{
    $obj = new Foo();
    $obj->set('foo', 'bar');
    $obj2 = new Foo();
    $obj2->setFoo('bar');

    $this->assertEquals($obj, $obj2);
}
dogstarTest commented 10 years ago

@nkt I try to run the tests in 3 ways as bellow:

  1. SPLIT PHPUNIT TEST BUT CORE DUMP //test_MagicDi_Spilt.php <?php class MagicDiSpilt_Test extends PHPUnit_Framework_Testcase { public function testMagicMethod() { $di = new Dogstar\MagicDi();

    $di->setName('phalcon');
    $this->assertEquals('phalcon', $di->getName());

    } }

phpunit ./test_MagicDi_Spilt.php PHPUnit 4.0.17 by Sebastian Bergmann.

Segmentation fault (core dumped)

  1. NOT SPLIT PHPUNIT TEST PASS! (NOT PASS before) //test_MagicDi_NotSpilt.php <?php

class MagicDiNotSpilt_Test extends PHPUnit_Framework_Testcase { public $fdi;

public function setUp()
{
    $this->fdi = new Dogstar\MagicDi();
}

public function testMagicMethod()
{
    $di = $this->fdi;

    $di->setName('phalcon');
    $this->assertEquals('phalcon', $di->getName());
}

}

  1. NOT THE WAY! <?php

$di = new Dogstar\MagicDi();

$di->setName('phalcon'); var_dump($di->getName());

NULL output!!!

nkt commented 10 years ago

Please read https://help.github.com/articles/github-flavored-markdown, and wrap your code by right tags for highlighting.

nkt commented 10 years ago

About your issue, please show generated C code for methods __call and set, thanks for your report

dogstarTest commented 10 years ago

Thanks @nkt , I will attach the C code later. And I think it is not about the phpunit business, if I write the DI class with PHP, it will pass all the tests, no matter how I use or test the code.

nkt commented 10 years ago

@dogstarTest I see. But you use unit tests, as functional tests. It's ok, but you should understand it. Also, I know, that phpunit often doesn't help catch bugs, because it's creates many magic with your code e.g. temporary files with wrappers. Also the best solution - create pull request with test, or add repository link with code, produced bugs. Thanks for understanding!

dogstarTest commented 10 years ago
PHP_METHOD(Dogstar_MagicDi, __call) {

    int ZEPHIR_LAST_CALL_STATUS;
    zephir_nts_static zephir_fcall_cache_entry *_2 = NULL, *_4 = NULL;
    zval *name, *params, *prefix = NULL, _0 = zval_used_for_init, _1 = zval_used_for_init, *key = NULL, *_3 = NULL, *value = NULL, *_5, *_6, *_7;

    ZEPHIR_MM_GROW();
    zephir_fetch_params(1, 2, 0, &name, &params);

    ZEPHIR_INIT_VAR(value);
    ZVAL_NULL(value);

    ZEPHIR_SINIT_VAR(_0);
    ZVAL_LONG(&_0, 0);
    ZEPHIR_SINIT_VAR(_1);
    ZVAL_LONG(&_1, 3);
    ZEPHIR_CALL_FUNCTION(&prefix, "substr", &_2, name, &_0, &_1);
    zephir_check_call_status();
    ZEPHIR_SINIT_NVAR(_0);
    ZVAL_LONG(&_0, 3);
    ZEPHIR_SINIT_NVAR(_1);
    ZVAL_LONG(&_1, zephir_fast_strlen_ev(name));
    ZEPHIR_CALL_FUNCTION(&_3, "substr", &_2, name, &_0, &_1);
    zephir_check_call_status();
    ZEPHIR_CALL_FUNCTION(&key, "lcfirst", &_4, _3);
    zephir_check_call_status();
    ZEPHIR_OBS_VAR(value);
    zephir_array_isset_long_fetch(&value, params, 0, 0 TSRMLS_CC);
    ZEPHIR_INIT_VAR(_5);
    ZEPHIR_CONCAT_SVSVSVS(_5, "#__call# method = ", prefix, "; key = ", key, "; value = ", value, "\n ");
    zend_print_zval(_5, 0);
    if (ZEPHIR_IS_STRING(prefix, "get")) {
        ZEPHIR_RETURN_CALL_METHOD(this_ptr, "get", NULL, key, value);
        zephir_check_call_status();
        RETURN_MM();
    }
    if (ZEPHIR_IS_STRING(prefix, "set")) {
        ZEPHIR_CALL_METHOD(NULL, this_ptr, "set", NULL, key, value);
        zephir_check_call_status();
        RETURN_MM_NULL();
    }
    ZEPHIR_INIT_VAR(_6);
    object_init_ex(_6, zend_exception_get_default(TSRMLS_C));
    ZEPHIR_INIT_VAR(_7);
    ZEPHIR_CONCAT_SVS(_7, "Call to undefined method Di::", name, "()");
    ZEPHIR_CALL_METHOD(NULL, _6, "__construct", NULL, _7);
    zephir_check_call_status();
    zephir_throw_exception_debug(_6, "dogstar/MagicDi.zep", 78 TSRMLS_CC);
    ZEPHIR_MM_RESTORE();
    return;

}
dogstarTest commented 10 years ago

PHP_METHOD(Dogstar_MagicDi, set) {

    HashTable *_3;
    HashPosition _2;
    int ZEPHIR_LAST_CALL_STATUS;
    zval *key, *value, *_0, *kk = NULL, *vv = NULL, *_1, **_4, *_5 = NULL;

    ZEPHIR_MM_GROW();
    zephir_fetch_params(1, 2, 0, &key, &value);

    ZEPHIR_INIT_VAR(_0);
    ZEPHIR_CONCAT_SVSVS(_0, "#set# key = ", key, "; value = ", value, "\n");
    zend_print_zval(_0, 0);
    ZEPHIR_CALL_METHOD(NULL, this_ptr, "_checkkey", NULL, key);
    zephir_check_call_status();
    zephir_update_property_array(this_ptr, SL("_data"), key, value TSRMLS_CC);
    php_printf("#data after set# ");
    _1 = zephir_fetch_nproperty_this(this_ptr, SL("_data"), PH_NOISY_CC);
    zephir_is_iterable(_1, &_3, &_2, 0, 0);
    for (
      ; zephir_hash_get_current_data_ex(_3, (void**) &_4, &_2) == SUCCESS
      ; zephir_hash_move_forward_ex(_3, &_2)
    ) {
        ZEPHIR_GET_HMKEY(kk, _3, _2);
        ZEPHIR_GET_HVALUE(vv, _4);
        ZEPHIR_INIT_LNVAR(_5);
        ZEPHIR_CONCAT_VSVS(_5, kk, " -> ", value, "\t");
        zend_print_zval(_5, 0);
    }
    php_printf("\n");
    ZEPHIR_MM_RESTORE();

}
dogstarTest commented 10 years ago

Thanks @nkt again! You are so nice and smart. As you know, English is not my first language, maybe I will show my option not correctly, never mind. __call and set function in C have been attached.

dogstarTest commented 10 years ago

Another fatal question about core dump. When I try to new another instance and use it, it will crash.

<?php
$di = new Dogstar\MagicDi();
$di->setName('201407');
var_dump($di->getName());

$di2 = new Dogstar\MagicDi();
$di2->setName('201408');
var_dump($di2->getName());
strace php ./test_MagicDi_Spilt_WithoutUnit.php 

--- SIGSEGV (Segmentation fault) @ 0 (0) ---
+++ killed by SIGSEGV (core dumped) +++
Segmentation fault (core dumped)

At first, I wonder any class with __call method will run crash when create serveral instance. But I found it is not after I verify the point with code and test.