Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:56378
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain ralphschindler.com from 209.85.213.42 cause and error)
Message-ID: <4EC5924C.8000605@ralphschindler.com>
Date: Thu, 17 Nov 2011 17:01:32 -0600
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: Anthony Ferrara <ircmaxell@gmail.com>
CC: internals <internals@lists.php.net>
References: <4EC578C9.4000408@ralphschindler.com> <CAAyV7nHEdvWm8QhYoya2wKhA5T67QAy9MQiMc93TRhGt6tgmFg@mail.gmail.com>
In-Reply-To: <CAAyV7nHEdvWm8QhYoya2wKhA5T67QAy9MQiMc93TRhGt6tgmFg@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Changes to constructor signature checks
From: ralph@ralphschindler.com (Ralph Schindler)

Inline...

> Honestly, I think __construct should behave like any other method when
> specified abstract or via an interface.

Which is not fair to say because constructors are not like instance 
methods, they are in fact "special", and not just inside PHP (more on 
that below).

> I know you're thinking "But it's not an LSP violation in the
> constructor".  But my assertion to that is that it's a violation of
> the contract that the abstract method / interface defined.  If it's a
> violation of contract, I would expect to get a fatal error.  After
> all, that's what a contract is for if not to enforce.
>
> Now, you could make the argument that it's impossible to enforce a
> contract (via type-hinting/instanceof) since there's no object at that
> point.  So the contract isn't "signed".  And that's the *only* reason
> that I could understand for wanting to change it from a fatal error to
> ignored.

So, since PHP lets you do bad things in the first place (like have 
constructors and static methods in interfaces, and abstract ctors in 
abstract classes), we follow that up with another "bad" of breaking 
general LSP expectations of how things work?

Isn't this trying to make two wrongs make a right?

> Additionally, what about other magic methods?  Should they be handled
> separately?  I could understand a contract to enforce __invoke and
> __toString, but what about the rest?  Should they be specifiable in
> contract form (get/set/call/etc)?

Again, CTOR is not "special" b/c it's a PHP "magic method" its "special" 
because it is shares qualities of being a static method as well as an 
instance method.

The rest of the __magic() methods by and large are purely instance 
methods (part of the instance API).  Having them inside an interface is 
completely logical.  (BTW, these methods can be considered 
engine-level/promoted duck typing since you don't have to implement an 
interface for them to do what they do, but that's not important here)

> So, actually I'm not so sure on that myself.  The Only use case I can
> think of I've used here:
> https://github.com/ircmaxell/PHP-CryptLib/blob/master/lib/CryptLib/Core/AbstractFactory.php#L51
>
> Basically, it's an abstract factory that checks if a class name
> implements an interface before it will "register" the class.  This is
> useful for implementing proxy patterns and the such.  So we don't

Lets look at that, the proxy pattern that is.  This is no longer valid 
usage in 5.4:

<?php

abstract class Person {
     protected $name = null;
     abstract public function __construct();
     public function setName($name)
     {
         $this->name = $name;
     }
     public function getName()
     {
         return $this->name;
     }
}

class NamedAgedPerson extends Person {
     protected $age;
     public function __construct($name, $age) { // COMMENT OUT HERE
         $this->name = $name;
         $this->age  = $age;
     }
}

class ProxyNamedAgedPerson extends NamedAgedPerson {
     public $proxy = null;
     public function __construct(NamedAgedPerson $person)
     {
         $this->proxy = $person;
     }
     public function setAge($age)
     {
         $this->proxy->age = $age;
     }
}

$np = new NamedAgedPerson('foo', 33);
$ap = new ProxyNamedAgedPerson($np);
$ap->setAge(40);
var_dump($ap);




Let's ignore the fact that it was not a very good idea to have the ctor 
in the abstract person in the first place.

In the above example, it is important to remember that an instance of 
NamedAgedPerson behaves exactly any other instance of Person in any 
situation where "instanceof Person" is the type check constraint.

There are three things wrong with this scenario.

First, if I consume a system that has practiced this abstract ctor in 
abstract class thing, I cannot create valid subtypes on my own in PHP 
5.4.  The above will throw an E_FATAL.

Secondly, (AND WORSE) is that by simply removing the abstract keyword 
from the abstract classes __construct(), I get completely different 
behavior, everything works without E_FATAL.

Thirdly, (EVEN WORSE!), is that if I comment out the $name, $age part of 
the NamedAgedPerson (see: COMMENT OUT HERE above), no E_FATAL is thrown 
on *grandchildren* who violate this abstract ctor.

So, if we choose to do this wrong+wrong=~right thing, we should at least 
do it correctly/consistently all the way through the inheritance type 
hierarchy.  Don't you think?

> And I do lean on the side that the contract is the contract, and if
> you change (overload) a method, you're breaking the contract.  And if
> you specify the constructor in the contract (you don't have to), you
> should be bound to that contract...
>
> So I think 5.4's behavior is the proper one.  I won't be upset if it
> changes, but if it does I think the rest of the magic methods (two
> mentioned withstanding) should change as well...

Of course, this argument is mostly pedantic to be honest, as I'd be 
instructing people to never attempt to put abstract ctors in abstract 
classes.  I won't be doing that either, but, the argument is largely 
that the engine shouldn't be doing these kind of things to begin off with.

Also, I'd has to see this kind of decision impacting future decisions 
(like this) b/c it was "ratified" in 5.4.

-ralph