Hi, 2010/6/28 Johannes Schlüter <johan...@schlueters.de>
> Hi, > > On Sat, 2010-06-26 at 22:05 +0000, Felipe Pena wrote: > > felipe Sat, 26 Jun 2010 22:05:13 +0000 > > > > Revision: http://svn.php.net/viewvc?view=revision&revision=300770 > > > > Log: > > - Fixed bug #51421 (Abstract __construct constructor argument list not > enforced) > > > > Bug: http://bugs.php.net/51421 (Closed) Abstract __construct constructor > argument list not enforced > > Won't this break compatibility? > > I'd say the change is logically correct but not sure we should break BC > there during RC. > At the risk of starting some political debate, I believe that the current state of prototype checks make close to no sense in PHP, for 4 reasons: 1) arguments can be gathered dynamically in the function, using func_get_args. For that reason, PHP gracefully allow a call with too many arguments. 2) a child method should be allowed to define type hints in a contra-variant manner (currently, it's invariant) 3) a child method should be allowed to return a ref even if the parent method is not defined to do so. (returning a ref is an additional constraint, and following co-variance of return values, it should be allowed). The basic example of this annoyance is: abstract class A implements ArrayAcces { public function &__offsetGet($name) { ... } } 4) all in all: it shouldn't throw a fatal error, those are not critical situations for the engine. I'd like to propose to relax such checks, by either allowing more (e.g. 1, 2 and 3) which can be painful and complicated, or simply removing those checks. Best, > > johannes > > > Changed paths: > > A php/php-src/branches/PHP_5_2/Zend/tests/bug51421.phpt > > U php/php-src/branches/PHP_5_2/Zend/zend_compile.c > > A php/php-src/branches/PHP_5_3/Zend/tests/bug51421.phpt > > U php/php-src/branches/PHP_5_3/Zend/zend_compile.c > > A php/php-src/trunk/Zend/tests/bug51421.phpt > > U php/php-src/trunk/Zend/zend_compile.c > > > > Added: php/php-src/branches/PHP_5_2/Zend/tests/bug51421.phpt > > =================================================================== > > --- php/php-src/branches/PHP_5_2/Zend/tests/bug51421.phpt > (rev 0) > > +++ php/php-src/branches/PHP_5_2/Zend/tests/bug51421.phpt 2010-06-26 > 22:05:13 UTC (rev 300770) > > @@ -0,0 +1,18 @@ > > +--TEST-- > > +Bug #51421 (Abstract __construct constructor argument list not enforced) > > +--FILE-- > > +<?php > > + > > +class ExampleClass {} > > + > > +abstract class TestInterface { > > + abstract public function __construct(ExampleClass $var); > > +} > > + > > +class Test extends TestInterface { > > + public function __construct() {} > > +} > > + > > +?> > > +--EXPECTF-- > > +Fatal error: Declaration of Test::__construct() must be compatible with > that of TestInterface::__construct() in %s on line %d > > > > > > Property changes on: > php/php-src/branches/PHP_5_2/Zend/tests/bug51421.phpt > > ___________________________________________________________________ > > Added: svn:keywords > > + Id Rev Revision > > Added: svn:eol-style > > + native > > > > Modified: php/php-src/branches/PHP_5_2/Zend/zend_compile.c > > =================================================================== > > --- php/php-src/branches/PHP_5_2/Zend/zend_compile.c 2010-06-26 21:29:56 > UTC (rev 300769) > > +++ php/php-src/branches/PHP_5_2/Zend/zend_compile.c 2010-06-26 22:05:13 > UTC (rev 300770) > > @@ -2009,13 +2009,20 @@ > > { > > zend_uint i; > > > > - /* If it's a user function then arg_info == NULL means we don't > have any parameters but we still need to do the arg number checks. We are > only willing to ignore this for internal functions because extensions don't > always define arg_info. */ > > + /* If it's a user function then arg_info == NULL means we don't > have any parameters but > > + * we still need to do the arg number checks. We are only willing > to ignore this for internal > > + * functions because extensions don't always define arg_info. > > + */ > > if (!proto || (!proto->common.arg_info && proto->common.type != > ZEND_USER_FUNCTION)) { > > return 1; > > } > > > > - /* Checks for constructors only if they are declared in an > interface */ > > - if ((fe->common.fn_flags & ZEND_ACC_CTOR) && > !(proto->common.scope->ce_flags & ZEND_ACC_INTERFACE)) { > > + /* Checks for constructors only if they are declared in an > interface, > > + * or explicitly marked as abstract > > + */ > > + if ((fe->common.fn_flags & ZEND_ACC_CTOR) > > + && ((proto->common.scope->ce_flags & ZEND_ACC_INTERFACE) == > 0 > > + && (proto->common.fn_flags & ZEND_ACC_ABSTRACT) == > 0)) { > > return 1; > > } > > > > > > Added: php/php-src/branches/PHP_5_3/Zend/tests/bug51421.phpt > > =================================================================== > > --- php/php-src/branches/PHP_5_3/Zend/tests/bug51421.phpt > (rev 0) > > +++ php/php-src/branches/PHP_5_3/Zend/tests/bug51421.phpt 2010-06-26 > 22:05:13 UTC (rev 300770) > > @@ -0,0 +1,18 @@ > > +--TEST-- > > +Bug #51421 (Abstract __construct constructor argument list not enforced) > > +--FILE-- > > +<?php > > + > > +class ExampleClass {} > > + > > +abstract class TestInterface { > > + abstract public function __construct(ExampleClass $var); > > +} > > + > > +class Test extends TestInterface { > > + public function __construct() {} > > +} > > + > > +?> > > +--EXPECTF-- > > +Fatal error: Declaration of Test::__construct() must be compatible with > that of TestInterface::__construct() in %s on line %d > > > > > > Property changes on: > php/php-src/branches/PHP_5_3/Zend/tests/bug51421.phpt > > ___________________________________________________________________ > > Added: svn:keywords > > + Id Rev Revision > > Added: svn:eol-style > > + native > > > > Modified: php/php-src/branches/PHP_5_3/Zend/zend_compile.c > > =================================================================== > > --- php/php-src/branches/PHP_5_3/Zend/zend_compile.c 2010-06-26 21:29:56 > UTC (rev 300769) > > +++ php/php-src/branches/PHP_5_3/Zend/zend_compile.c 2010-06-26 22:05:13 > UTC (rev 300770) > > @@ -2532,13 +2532,20 @@ > > { > > zend_uint i; > > > > - /* If it's a user function then arg_info == NULL means we don't > have any parameters but we still need to do the arg number checks. We are > only willing to ignore this for internal functions because extensions don't > always define arg_info. */ > > + /* If it's a user function then arg_info == NULL means we don't > have any parameters but > > + * we still need to do the arg number checks. We are only willing > to ignore this for internal > > + * functions because extensions don't always define arg_info. > > + */ > > if (!proto || (!proto->common.arg_info && proto->common.type != > ZEND_USER_FUNCTION)) { > > return 1; > > } > > > > - /* Checks for constructors only if they are declared in an > interface */ > > - if ((fe->common.fn_flags & ZEND_ACC_CTOR) && > !(proto->common.scope->ce_flags & ZEND_ACC_INTERFACE)) { > > + /* Checks for constructors only if they are declared in an > interface, > > + * or explicitly marked as abstract > > + */ > > + if ((fe->common.fn_flags & ZEND_ACC_CTOR) > > + && ((proto->common.scope->ce_flags & ZEND_ACC_INTERFACE) == > 0 > > + && (proto->common.fn_flags & ZEND_ACC_ABSTRACT) == > 0)) { > > return 1; > > } > > > > > > Added: php/php-src/trunk/Zend/tests/bug51421.phpt > > =================================================================== > > --- php/php-src/trunk/Zend/tests/bug51421.phpt > (rev 0) > > +++ php/php-src/trunk/Zend/tests/bug51421.phpt 2010-06-26 22:05:13 > UTC (rev 300770) > > @@ -0,0 +1,18 @@ > > +--TEST-- > > +Bug #51421 (Abstract __construct constructor argument list not enforced) > > +--FILE-- > > +<?php > > + > > +class ExampleClass {} > > + > > +abstract class TestInterface { > > + abstract public function __construct(ExampleClass $var); > > +} > > + > > +class Test extends TestInterface { > > + public function __construct() {} > > +} > > + > > +?> > > +--EXPECTF-- > > +Fatal error: Declaration of Test::__construct() must be compatible with > that of TestInterface::__construct() in %s on line %d > > > > > > Property changes on: php/php-src/trunk/Zend/tests/bug51421.phpt > > ___________________________________________________________________ > > Added: svn:keywords > > + Id Rev Revision > > Added: svn:eol-style > > + native > > > > Modified: php/php-src/trunk/Zend/zend_compile.c > > =================================================================== > > --- php/php-src/trunk/Zend/zend_compile.c 2010-06-26 21:29:56 UTC > (rev 300769) > > +++ php/php-src/trunk/Zend/zend_compile.c 2010-06-26 22:05:13 UTC > (rev 300770) > > @@ -2909,13 +2909,20 @@ > > { > > zend_uint i; > > > > - /* If it's a user function then arg_info == NULL means we don't > have any parameters but we still need to do the arg number checks. We are > only willing to ignore this for internal functions because extensions don't > always define arg_info. */ > > + /* If it's a user function then arg_info == NULL means we don't > have any parameters but > > + * we still need to do the arg number checks. We are only willing > to ignore this for internal > > + * functions because extensions don't always define arg_info. > > + */ > > if (!proto || (!proto->common.arg_info && proto->common.type != > ZEND_USER_FUNCTION)) { > > return 1; > > } > > > > - /* Checks for constructors only if they are declared in an > interface */ > > - if ((fe->common.fn_flags & ZEND_ACC_CTOR) && > !(proto->common.scope->ce_flags & ZEND_ACC_INTERFACE)) { > > + /* Checks for constructors only if they are declared in an > interface, > > + * or explicitly marked as abstract > > + */ > > + if ((fe->common.fn_flags & ZEND_ACC_CTOR) > > + && ((proto->common.scope->ce_flags & ZEND_ACC_INTERFACE) == > 0 > > + && (proto->common.fn_flags & ZEND_ACC_ABSTRACT) == > 0)) { > > return 1; > > } > > > > > > -- > > PHP CVS Mailing List (http://www.php.net/) > > To unsubscribe, visit: http://www.php.net/unsub.php > > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > -- Etienne Kneuss http://www.colder.ch
