Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:47711
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain smashlabs.com from 67.15.58.61 cause and error)
Message-ID: <4BB4EC2F.1020502@smashlabs.com>
Date: Thu, 01 Apr 2010 13:55:43 -0500
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9) Gecko/20061207 Thunderbird/1.5.0.9 Mnenhy/0.7.4.666
MIME-Version: 1.0
To: internals <internals@lists.php.net>
Content-Type: multipart/mixed;
 boundary="------------040902030604090205090509"
Subject: On constructors: BC Break and Class compiler Improvements
From: ralph@smashlabs.com (Ralph Schindler)

--------------040902030604090205090509
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hey Internals,

In our work converting the Zend Framework library to namespaces, we came 
across some inconsistencies in constructor namings.

Clearly, with namespace support making class names shorter, we come back 
full circle where some class names collide with reserved words, thus we 
are forced to become creative with class names:

What was once:

   class Foo_Bar_Array {}

would become (1 to 1 mapping):

   namespace Foo\Bar; class Array {}

but must become

   namespace Foo\Bar; class ArrayBar {} (or ArrayObj, ArrayClass, etc)

We can work around these limitations.

With that in mind though, this is the bigger challenge we are faced with...


The following seems, to me, to be a BC break with regards to notices and 
E_STRICT usage:

   namespace Foo\Bar;
   class Filter {
     public function __construct() { /* construct stuff */ }
     public function filter($value) { /* return filtered */ }
   }

Produces:

   PHP Strict Standards:  Redefining already defined constructor for
   class Zend\Filter\Filter in [snip file] on line [snip line]


This worked in 5.2.x.
This worked in 5.3.0.
This stopped working in 5.3.1


Moreover, I do think that this is NOT the correct behavior.  While I 
know somewhere in the history of PHP we had an engine that preferred 
class names as constructor names, but thankfully- those days (one could 
only hope) are gone.

IMO, the engine should be backwards compatible as best as possbile in 
the 5.x series and should (with the respect of PHP4 tendencies), reward 
programmers using proper PHP5 constructs, like __construct.

(While I hate to use the english language as a crutch for the argument.. 
PHP is after all written in english, ... it is considered a best 
practice do have class names as nouns and methods as verbs.  Since there 
are a lot of cases where the same words are both the noun and verb.. the 
verb being the method being acted on in the class, the noun - combined 
with the shorter class names due to namespaces, I can see this becoming 
a bigger problem as more people embrace namespaces.)

Furthermore, static functions of the same name should not be marked as 
constructors by the engine in any situation (is there some BC we are 
keeping here? b/c I cannot see the use case.)




Attached is a patch that fixes this behavior (against PHP_5_3).  What it 
does:

   * If __construct is declared before a method of the same name as the 
class, you WILL NOT get a notice

   * If a method of the same name as the class appears before 
__construct(), you WILL get a notice, as the current behavior.

   (The idea here is that if you define __construct before anything 
else, as most people do, it will take priority and allow you to use the 
method name of the class name below that as normal)

   * Static methods of the same name as the class will NOT be marked as 
constructors (they currently are).  This might invalidate the check done 
inside zend_do_end_class_declaration() that checks to see if the 
constructor is static.  (Perhaps that first check becomes dead code?)



I am not sure where the current behavior came from. I looks like it got 
merged into 5_2_x but then backed out and merged into 5_3_x somewhere 
along the line after 5.3.0 was released.

In any case, considering we are not gonna get a context aware parser 
that will allow us to use reserved words for symbols like classes and 
functions, it would be nice to not have this seemingly artificial 
limitation on having method names that do not match the class name.

Thanks,
Ralph Schindler

References:

   http://bugs.php.net/bug.php?id=35634
   http://bugs.php.net/bug.php?id=48215

--------------040902030604090205090509
Content-Type: text/plain; x-mac-type="0"; x-mac-creator="0";
 name="fix-constructor-e_strict.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="fix-constructor-e_strict.diff"

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 13b6c55..b7cd7b0 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -1285,13 +1285,11 @@ void zend_do_begin_function_declaration(znode *function_token, znode *function_n
 			zend_str_tolower_copy(short_class_lcname, short_class_name, short_class_name_length);
 			/* Improve after RC: cache the lowercase class name */
 
-			if ((short_class_name_length == name_len) && (!memcmp(short_class_lcname, lcname, name_len))) {
-				if (CG(active_class_entry)->constructor) {
-					zend_error(E_STRICT, "Redefining already defined constructor for class %s", CG(active_class_entry)->name);
-				} else {
+			if ((short_class_name_length == name_len) && (!memcmp(short_class_lcname, lcname, name_len)) && ((fn_flags & ZEND_ACC_STATIC) == 0)) {
+				if (!CG(active_class_entry)->constructor) {
 					CG(active_class_entry)->constructor = (zend_function *) CG(active_op_array);
 				}
-			} else if ((name_len == sizeof(ZEND_CONSTRUCTOR_FUNC_NAME)-1) && (!memcmp(lcname, ZEND_CONSTRUCTOR_FUNC_NAME, sizeof(ZEND_CONSTRUCTOR_FUNC_NAME)))) {
+			} else if ((name_len == sizeof(ZEND_CONSTRUCTOR_FUNC_NAME)-1) && (!memcmp(lcname, ZEND_CONSTRUCTOR_FUNC_NAME, sizeof(ZEND_CONSTRUCTOR_FUNC_NAME))) && ((fn_flags & ZEND_ACC_STATIC) == 0)) {
 				if (CG(active_class_entry)->constructor) {
 					zend_error(E_STRICT, "Redefining already defined constructor for class %s", CG(active_class_entry)->name);
 				}

--------------040902030604090205090509--