Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extends Tarantool in PHP 7.2 bug #135

Closed
eugenchenko opened this issue Aug 1, 2018 · 8 comments
Closed

extends Tarantool in PHP 7.2 bug #135

eugenchenko opened this issue Aug 1, 2018 · 8 comments
Labels
bug Something isn't working

Comments

@eugenchenko
Copy link

eugenchenko commented Aug 1, 2018

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

@bigbes
Copy link
Contributor

bigbes commented Aug 14, 2018

Should be fixed in 108b658

@rybakit
Copy link
Collaborator

rybakit commented Nov 6, 2018

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.

@Totktonada Totktonada added the bug Something isn't working label Mar 23, 2020
@Totktonada
Copy link
Member

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

Totktonada added a commit that referenced this issue Mar 23, 2020
The proper initialization is important for correct work of an inherited
class. I cannot provide exact description what was broken and becomes
right, but at least the test case with inherited class with a property
starts to work correctly. Some online materials strongly suggest to call
object_properties_init() function in a create_object handler, see [1],
[2], [3].

The problem was catched at the existing test suite on mocking tests when
phpunit was updated to 6.5.14 (now it is 4.8.24) and appropriate changes
were made for the tests (say, using of createMock() instead of
getMock()). So this commit is prerequisite to run the test suite on more
fresh phpunit, which is necessary to test the connector on php-7.1+. The
changes for php-7.1+ will land in future commits.

It seems the problem was introduced in [4].

[1]: https://wiki.php.net/internals/engine/objects
[2]: http://www.phpinternalsbook.com/php5/classes_objects/custom_object_storage.html
[3]: https://phabricator.wikimedia.org/T59292
[4]: 9f5a282 ('updated PHP7 implementation')

Fixes #135.
Totktonada added a commit that referenced this issue Mar 23, 2020
The proper initialization is important for correct work of an inherited
class. I cannot provide exact description what was broken and becomes
right, but at least the test case with inherited class with a property
starts to work correctly. Some online materials strongly suggest to call
object_properties_init() function in a create_object handler, see [1],
[2], [3].

The problem was catched at the existing test suite on mocking tests when
phpunit was updated to 6.5.14 (now it is 4.8.24) and appropriate changes
were made for the tests (say, using of createMock() instead of
getMock()). So this commit is prerequisite to run the test suite on more
fresh phpunit, which is necessary to test the connector on php-7.1+. The
changes for php-7.1+ will land in future commits.

It seems the problem was introduced in [4].

[1]: https://wiki.php.net/internals/engine/objects
[2]: http://www.phpinternalsbook.com/php5/classes_objects/custom_object_storage.html
[3]: https://phabricator.wikimedia.org/T59292
[4]: 9f5a282 ('updated PHP7 implementation')

Fixes #135.
@Totktonada Totktonada added this to the Support PHP 7.* milestone Mar 23, 2020
@rybakit
Copy link
Collaborator

rybakit commented Mar 23, 2020

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.

@eugenchenko
Copy link
Author

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

@Totktonada
Copy link
Member

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).

@rybakit
Copy link
Collaborator

rybakit commented Mar 23, 2020

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.

Totktonada added a commit that referenced this issue Mar 25, 2020
The proper initialization is important for correct work of an inherited
class. I cannot provide exact description what was broken and becomes
right, but at least the test case with inherited class with a property
starts to work correctly. Some online materials strongly suggest to call
object_properties_init() function in a create_object handler, see [1],
[2], [3].

The problem was catched at the existing test suite on mocking tests when
phpunit was updated to 6.5.14 (now it is 4.8.24) and appropriate changes
were made for the tests (say, using of createMock() instead of
getMock()). So this commit is prerequisite to run the test suite on more
fresh phpunit, which is necessary to test the connector on php-7.1+. The
changes for php-7.1+ will land in future commits.

It seems the problem was introduced in [4].

[1]: https://wiki.php.net/internals/engine/objects
[2]: http://www.phpinternalsbook.com/php5/classes_objects/custom_object_storage.html
[3]: https://phabricator.wikimedia.org/T59292
[4]: 9f5a282 ('updated PHP7 implementation')

Fixes #135.
Totktonada added a commit that referenced this issue Mar 26, 2020
The proper initialization is important for correct work of an inherited
class. I cannot provide exact description what was broken and becomes
right, but at least the test case with inherited class with a property
starts to work correctly. Some online materials strongly suggest to call
object_properties_init() function in a create_object handler, see [1],
[2], [3].

The problem was catched at the existing test suite on mocking tests when
phpunit was updated to 6.5.14 (now it is 4.8.24) and appropriate changes
were made for the tests (say, using of createMock() instead of
getMock()). So this commit is prerequisite to run the test suite on more
fresh phpunit, which is necessary to test the connector on php-7.1+. The
changes for php-7.1+ will land in future commits.

It seems the problem was introduced in [4].

[1]: https://wiki.php.net/internals/engine/objects
[2]: http://www.phpinternalsbook.com/php5/classes_objects/custom_object_storage.html
[3]: https://phabricator.wikimedia.org/T59292
[4]: 9f5a282 ('updated PHP7 implementation')

Fixes #135.
Totktonada added a commit that referenced this issue Mar 28, 2020
The proper initialization is important for correct work of an inherited
class. I cannot provide exact description what was broken and becomes
right, but at least the test case with inherited class with a property
starts to work correctly. Some online materials strongly suggest to call
object_properties_init() function in a create_object handler, see [1],
[2], [3].

The problem was catched at the existing test suite on mocking tests when
phpunit was updated to 6.5.14 (now it is 4.8.24) and appropriate changes
were made for the tests (say, using of createMock() instead of
getMock()). So this commit is prerequisite to run the test suite on more
fresh phpunit, which is necessary to test the connector on php-7.1+. The
changes for php-7.1+ will land in future commits.

It seems the problem was introduced in [4].

[1]: https://wiki.php.net/internals/engine/objects
[2]: http://www.phpinternalsbook.com/php5/classes_objects/custom_object_storage.html
[3]: https://phabricator.wikimedia.org/T59292
[4]: 9f5a282 ('updated PHP7 implementation')

Fixes #135.
@Totktonada
Copy link
Member

Totktonada commented Mar 28, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants