Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:35863
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain cschneid.com from 195.141.85.117 cause and error)
Message-ID: <47C84070.2030708@cschneid.com>
Date: Fri, 29 Feb 2008 18:27:12 +0100
User-Agent: Thunderbird 1.5.0.12 (X11/20060911)
MIME-Version: 1.0
To: Marcus Boerger <helly@php.net>
CC: Lukas Kahwe Smith <mls@pooteeweet.org>, 
 internals Mailing List <internals@lists.php.net>
Content-Type: multipart/mixed;
 boundary="------------060107080605000908030807"
Subject: OO Parameter Checking
From: cschneid@cschneid.com (Christian Schneider)

--------------060107080605000908030807
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Marcus Boerger wrote:
>   you still cannot ignore basic inheritance or reuse rules. Protocols
> have to be respected -> E_FATAL, fix your code.

I have several comments (plus patches) to this:

1) The current checks are IMHO too strict regarding default values for
parameters: An inheriting class can add default values to a parameter
without breaking the protocol:
class A { function foo($x) { ... } }
class B extends A { function foo($x = 42) { ... } }
should be allowed. Only if there are fewer parameters in B::foo or if
B::foo has more *required* parameters than A::foo the protocol is
broken. Otherwise every instance of B can be passed to something
expecting A.
The attached paramcheck_relaxed.patch.txt for HEAD changes this.

2) The current checks are IMHO too strict regarding static functions:
Static functions are not part of an instance's protocol (they can not be
passed around) and should be left out of the check the same way
constructors are ignored. The use case for this is a factory method:
class A { static function factory($size) { ... } }
class B extends A { static function factory($size, $color) { ... } }
The attached paramcheck_relaxed.patch.txt for HEAD changes this.

3) I guess more controversial is the change from E_STRICT to
E_COMPILE_ERROR for PHP 6 regarding these checks. While you consider
such OO code as broken I would highly appreciate it if you recognize
that other people have other coding standards and/or old code to
maintain. Adhering to the
http://en.wikipedia.org/wiki/Liskov_substitution_principle is something
strict OO coders want to do. And they are using E_STRICT anyway, so I
see no reason to force everybody else to change their code, so it should
be left a simple E_STRICT IMHO.
The attached paramcheck_e_strict.patch for HEAD changes this back to the
old behaviour.

If paramcheck_relaxed.patch.txt gets accepted I'd volunteer to back-port
it to 5.x as well.

Regards,
- Chris

--------------060107080605000908030807
Content-Type: text/plain;
 name="paramcheck_relaxed.patch.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="paramcheck_relaxed.patch.txt"

Index: Zend/zend_compile.c
===================================================================
RCS file: /repository/ZendEngine2/zend_compile.c,v
retrieving revision 1.804
diff -u -r1.804 zend_compile.c
--- Zend/zend_compile.c	23 Feb 2008 21:24:18 -0000	1.804
+++ Zend/zend_compile.c	29 Feb 2008 16:26:51 -0000
@@ -2477,12 +2477,12 @@
 	}
 
 	/* 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)) {
+	if ((fe->common.fn_flags & (ZEND_ACC_CTOR | ZEND_ACC_STATIC)) && !(proto->common.scope->ce_flags & ZEND_ACC_INTERFACE)) {
 		return 1;
 	}
 
 	/* check number of arguments */
-	if (proto->common.required_num_args != fe->common.required_num_args
+	if (proto->common.required_num_args < fe->common.required_num_args
 		|| proto->common.num_args > fe->common.num_args) {
 		return 0;
 	}

--------------060107080605000908030807
Content-Type: text/plain;
 name="paramcheck_e_strict.patch.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="paramcheck_e_strict.patch.txt"

Index: Zend/zend_compile.c
===================================================================
RCS file: /repository/ZendEngine2/zend_compile.c,v
retrieving revision 1.804
diff -u -r1.804 zend_compile.c
--- Zend/zend_compile.c	23 Feb 2008 21:24:18 -0000	1.804
+++ Zend/zend_compile.c	29 Feb 2008 17:23:45 -0000
@@ -2617,7 +2617,7 @@
 		child->common.prototype = parent->common.prototype ? parent->common.prototype : parent;
 	}
 
-	if (child->common.prototype) {
+	if (child->common.prototype && (child->common.prototype->common.fn_flags & ZEND_ACC_ABSTRACT)) {
 		if (!zend_do_perform_implementation_check(child, child->common.prototype TSRMLS_CC)) {
 			zend_error(E_COMPILE_ERROR, "Declaration of %v::%v() must be compatible with that of %v::%v()", ZEND_FN_SCOPE_NAME(child), child->common.function_name, ZEND_FN_SCOPE_NAME(child->common.prototype), child->common.prototype->common.function_name);
 		}

--------------060107080605000908030807--