[PHP-CVS] svn: /php/php-src/ branches/PHP_5_2/Zend/tests/bug51421.phpt branches/PHP_5_2/Zend/zend_compile.c branches/PHP_5_3/Zend/tests/bug51421.phpt branches/PHP_5_3/Zend/zend_compile.c trunk/Zend/tests/bug51421.phpt trunk/Zend/zen

14 years ago by Etienne Kneuss — view source

unread

Hi,

2010/6/28 Johannes Schlüter johannes@schlueters.de

Hi,

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:

arguments can be gathered dynamically in the function, using
func_get_args. For that reason, PHP gracefully allow a call with too many
arguments.
a child method should be allowed to define type hints in a contra-variant
manner (currently, it's invariant)
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) { ... } }
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/)
--

--
Etienne Kneuss
http://www.colder.ch

14 years ago by Stas Malyshev — view source

unread

Hi!

arguments can be gathered dynamically in the function, using
func_get_args. For that reason, PHP gracefully allow a call with too many
arguments.

Isn't it the case today?
Maybe also we can add some syntax like:
function foo($a, $b, ...) - where ... means "I'll deal with those
arguments myself, ignore any compatibility checks" so if the prototype
is foo($a,$b,$c) $a and $b would be checked but not $c and the function
is considered to have enough args.

a child method should be allowed to define type hints in a contra-variant
manner (currently, it's invariant)

This needs to be fixed.

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

This needs to be fixed too.

all in all: it shouldn't throw a fatal error, those are not critical
situations for the engine.

I'd say in general OO part of PHP seems to be much more strict and
Java-like than the rest. I'm not sure it's good. I'd demote all
situations where the engine has a way to proceed to at least warning level.

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.

I think we shouldn't remove them - we should fix them. It doesn't seem
to be too hard, IMHO.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

[PHP-CVS] svn: /php/php-src/ branches/PHP_5_2/Zend/tests/bug51421.phpt branches/PHP_5_2/Zend/zend_compile.c branches/PHP_5_3/Zend/tests/bug51421.phpt branches/PHP_5_3/Zend/zend_compile.c trunk/Zend/tests/bug51421.phpt trunk/Zend/zen

Added: php/php-src/branches/PHP_5_2/Zend/tests/bug51421.phpt

Modified: php/php-src/branches/PHP_5_2/Zend/zend_compile.c

Added: php/php-src/branches/PHP_5_3/Zend/tests/bug51421.phpt

Modified: php/php-src/branches/PHP_5_3/Zend/zend_compile.c

Added: php/php-src/trunk/Zend/tests/bug51421.phpt

Modified: php/php-src/trunk/Zend/zend_compile.c

I think we shouldn't remove them - we should fix them. It doesn't seem to be too hard, IMHO.

I think we shouldn't remove them - we should fix them. It doesn't seem
to be too hard, IMHO.