tarantool/tarantool-php

extends Tarantool in PHP 7.2 bug

eugenchenko opened this issue · 8 comments

Example #1:

test.php:
<?php

class A {

}

class B extends A
{
    private $_fcgfgfg = 'test';

    public function __construct()
    {
        print $this->_fcgfgfg;
    }
}

new B();

?>

$ php test.php
test

Example #2:

<?php

class B extends Tarantool
{
    private $_fcgfgfg = 'test';

    public function __construct()
    {
        print $this->_fcgfgfg;
    }
}

new B();

?>

$ php test.php
PHP Notice:  Undefined property: B::$_fcgfgfg in /home/eugenchenko/cryptobot/test.php on line 13

PHP 7.2.7-1+020180622080852.23+jessie1.gbpfd8e2e (cli) (built: Jun 22 2018 09:18:17) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
with Zend OPcache v7.2.7-1+020180622080852.23+jessie1.gbpfd8e2e, Copyright (c) 1999-2018, by Zend Technologies

Should be fixed in 108b658

I would even propose to mark the Tarantrool class as final to encourage using composition over inheritance. For example, that's what the MongoDb extension does: http://php.net/manual/en/book.mongodb.php.

Are there a reason to forbid inheritance? Aren't you have idea why MongoDB does?

Are there a reason to forbid inheritance? Aren't you have idea why MongoDB does?

I believe they just follow best practices. There are plenty of articles on the Internet explaining the pros and cons of composition over inheritance:

Personally, I can't think of any use case in which I would want to extend the Tarantool class. For the record, all classes in tarantool-php/client are final by design, and yet the client is very flexible in terms of extensibility and testability. Please note that all of this is my subjective opinion, which may differ from the opinion of active users of this extension.

i've tried to make a class with predefined auth credentials to use constructor without any params.

Thanks for the feedback!

My near goal is to revive the connector (php 7.* support, packages, bugfixes) and I surely will not introduce changes that may break backward compatibility (at least within two near releases).

I'll close the issue as the bug (with standard property initialization fix). We can consider this proposal again if we'll decide to evolve the connector further (don't sure now).

i've tried to make a class with predefined auth credentials to use constructor without any params.

@eugenchenko, I would use factory for that.

My near goal is to revive the connector (php 7.* support, packages, bugfixes) and I surely will not introduce changes that may break backward compatibility (at least within two near releases).

Fair enough.

Fixed in php7-v2 branch in 0.3.0-15-g905a70f. The branch will be switched to master in the scope of #137.