Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:72011
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain basereality.com from 209.85.160.171 cause and error)
MIME-Version: 1.0
In-Reply-To: <CE27EF9C.9917%seanc@fb.com>
References: <CE27EF9C.9917%seanc@fb.com>
Date: Sun, 2 Feb 2014 18:06:44 +0000
Message-ID: <CA+kxMuQ3nHzbycLezrTTb4Z-rhLBav0JZaG_AYdfBxMbr+wCQQ@mail.gmail.com>
To: Sean Cannella <seanc@fb.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] RFC: constructor argument promotion
From: danack@basereality.com (Dan Ackroyd)

Hi,

A little late to the game but here are some reasons why this RFC seems
be a bad idea to implement.


1) It makes it easier to write subtle bugs. e.g.

class A {
    function __construct(protected $foo1 = 'A1', private $foo2 = 'A2') {
    }
    //other methods that use $this->foo2
}

class B extends A {
    function __construct(protected $foo1 = 'B1', private $foo2 = 'B2') {
        parent::__contruct();
    }
    //other methods that use $this->foo2
}

Class A and class B each have their own private copy of $foo2, but
share the single instance of property $foo1. Which is quite surprising
behaviour.


2) For my reading of the proposed RFC, I don't think it can safely be
used for sub-classes. i.e. if you wrote this code in the new proposed
syntax.

class SQLConnection extends DBConnection {}

class A {
    function __construct(protected DBConnection $dbConnection){}
}

class B extends A {
    function __construct(protected SQLConnection $dbConnection){}
}

That would be equivalent to writing this code in the current PHP syntax.

class A {
    /** @var  DBConnection */
    protected $dbConnection;
    function __construct(DBConnection $dbConnection){}
}

class B extends A {
    /** @var  SQLConnection */
    protected $dbConnection;
    function __construct(SQLConnection $dbConnection){
        $this->dbConnection = $dbConnection;
    }
}

But that's not what you would want. You don't want class B to
redeclare the property with a different type. So either the RFC would
have to implement some complex rules for typehinting, or people would
have to avoid using this syntax in sub-classes.


3) It hinders future ability to make new features on properties, such
as the RFC https://wiki.php.net/rfc/propertygetsetsyntax-alternative-typehinting-syntax
Cramming the declarations of properties into the constructor make it
almost impossible for any future RFC that wants to affect how
properties are declared.

4) Makes code harder to read. Currently the comments for a property
will be written directly above that property, so if you want to read
the comment for a property in any decent IDE you can i) Click on the
property ii) Move your eyes up one line, and you're reading the
comment for the property.

This RFC moves the comments further away and so you have to scan up
lines of code to figure out where the properties comment is. Even if
the other negative features of this RFC weren't present, making code
harder to read and maintain defeats any gain from a bit less typing.

5) Breaks current ecosystem of annotations. People who use annotations
for properties would have to rewrite their tools to support a new
syntax. Although that is sometimes necessary, it is a negative point
for this RFC.


To summarise, to me this RFC seems to have lots of negative features.
The RFC currently under vote
(https://wiki.php.net/rfc/automatic_property_initialization#vote)
doesn't save quite as much typing as this RFC would, but doesn't
suffer from any of the above negative features and so seems far better
to me.

If people prefer this RFC it'd be nice if there was some suggestions
of how to fix these issues.

cheers
Dan


On Wed, Aug 7, 2013 at 8:47 PM, Sean Cannella <seanc@fb.com> wrote:
> Everyone -
>
> Hi! Since this is my first post to this list, I'll introduce myself:
> I'm an engineer who has been working on HipHop VM in New York for the last half year or so after a long time working at Microsoft on business software and services in multiple hemispheres.
>
> I wanted to get the PHP community's opinion on this feature. See the draft RFC and referenced implementation below:
>
> https://wiki.php.net/rfc/constructor-promotion
>
> What do you all think?
>
> Thanks,
> Sean Cannella | Facebook Eng
> If you are smart and persistent, you may find the truth. It is not always a reward.