Everything

php

Home » Project » Newsgroups » php.internals » Declaration of Bar::__construct() must be compatible with that of Foo::__construct() »

Declaration of Bar::construct() must be compatible with that of Foo::construct()

php.internals

Timm Friebe

22 years ago

thekid@friebes:~/devel/php/tests > cat inheritance.php
<?php
class Foo {
function __construct($foo) {
}
}

class Bar extends Foo {
function __construct($foo, $bar) {
// Add = NULL after $bar to make it work
}
}
?>
thekid@friebes:~/devel/php/tests > php-dev inheritance.php

Fatal error: Declaration of Bar::__construct() must be compatible with
that of Foo::__construct() in
/usr/home/thekid/devel/php/tests/inheritance.php on line 10

Is this really necessary?

- Timm

Marcus Börger

22 years ago

Hello Timm,

i had the same expirience today too. And also for me it makes not much
sense. The constructor shouldn't check inheritance rules. And as a
consequence maybe interfaces shouldn't allow constructors.

marcus

Wednesday, February 25, 2004, 11:36:57 PM, you wrote:

> thekid@friebes:~/devel/php/tests > cat inheritance.php
> <?php
>   class Foo {
>     function __construct($foo) {
>     }
>   }

>   class Bar extends Foo {
>     function __construct($foo, $bar) {
>       // Add = NULL after $bar to make it work
>     }
>   }

?>>

> thekid@friebes:~/devel/php/tests > php-dev inheritance.php

> Fatal error: Declaration of Bar::__construct() must be compatible with
> that of Foo::__construct() in
> /usr/home/thekid/devel/php/tests/inheritance.php on line 10

> Is this really necessary?

> - Timm

--
Best regards,
Marcus                            mailto:helly@php.net

Timm Friebe

22 years ago

On Wed, 2004-02-25 at 23:44, Marcus Boerger wrote:

> Hello Timm,
>
> i had the same expirience today too. And also for me it makes not much
> sense. The constructor shouldn't check inheritance rules.

Neither should other methods follow this. What if I want to add a
non-default parameter to an overriden method?

<?php
class Foo {
function connect($server) {
}
}

class Bar extends Foo {
function connect($server, $port) {
}
}
?>

I see where the problem comes from:

zend_do_perform_implementation_check() is called from
do_inherit_method_check() (both in zend_compile.c) which in turn is
called for inheritance *and* for interfaces. The behaviour is fully
desirable when implementing interfaces but not for regular inheritance.

- Timm

Marcus Börger

22 years ago

Hello Timm,

well for normal methods we must do that. The derived class must support the
same signature that the base class supports. In you example that would only
work if the derived method would have a default parameter for the additional
parameter:

<?php
class Foo {
function connect($server) {
}
}

class Bar extends Foo {
function connect($server, $port = NULL) {
}
}
?>

regards
marcus


Wednesday, February 25, 2004, 11:52:20 PM, you wrote:

> On Wed, 2004-02-25 at 23:44, Marcus Boerger wrote:
>> Hello Timm,
>>
>> i had the same expirience today too. And also for me it makes not much
>> sense. The constructor shouldn't check inheritance rules.

> Neither should other methods follow this. What if I want to add a
> non-default parameter to an overriden method?

> <?php
>   class Foo {
>     function connect($server) {
>     }
>   }

>   class Bar extends Foo {
>     function connect($server, $port) {
>     }
>   }

?>>

> I see where the problem comes from:

> zend_do_perform_implementation_check() is called from
> do_inherit_method_check() (both in zend_compile.c) which in turn is
> called for inheritance *and* for interfaces. The behaviour is fully
> desirable when implementing interfaces but not for regular inheritance.

> - Timm

--
Best regards,
Marcus                            mailto:helly@php.net

Adam Maccabee Trachtenberg

22 years ago

On Thu, 26 Feb 2004, Marcus Boerger wrote:

> well for normal methods we must do that. The derived class must support the
> same signature that the base class supports. In you example that would only
> work if the derived method would have a default parameter for the additional
> parameter:
>
> <?php
>   class Foo {
>     function connect($server) {
>     }
>   }
>
>   class Bar extends Foo {
>     function connect($server, $port = NULL) {
>     }
>   }
> ?>

When was this change made? I have a CVS check out from yesterday and
this is not enforced. This will breaks lots of PHP 4 code.

-adam

--
adam@trachtenberg.com
author of o'reilly's php cookbook
avoid the holiday rush, buy your copy today!

Derick Rethans

22 years ago

On Thu, 26 Feb 2004, Marcus Boerger wrote:

> Hello Timm,
>
> well for normal methods we must do that. The derived class must support the
> same signature that the base class supports. In you example that would only
> work if the derived method would have a default parameter for the additional
> parameter:

I think this is only annoying users for nothing... I really don't see a
good reason on why to enforce this.

Derick

Zeev Suraski

22 years ago

At 03:28 26/02/2004, Derick Rethans wrote:

>On Thu, 26 Feb 2004, Marcus Boerger wrote:
>
> > Hello Timm,
> >
> > well for normal methods we must do that. The derived class must support the
> > same signature that the base class supports. In you example that would only
> > work if the derived method would have a default parameter for the
> additional
> > parameter:
>
>I think this is only annoying users for nothing... I really don't see a
>good reason on why to enforce this.

PHP 5 has grown to depend on the is_a concept as a foundation building
block.  Allowing people to break is a bad idea, since it makes some of the
new features less powerful/reliable.  Preventing it now is the right thing
to do.  E_STRICT may be good for when compat mode's off.

Zeev

Timm Friebe

22 years ago

On Wed, 2004-02-25 at 23:52, Timm Friebe wrote:

> On Wed, 2004-02-25 at 23:44, Marcus Boerger wrote:
> > Hello Timm,
> >
> > i had the same expirience today too. And also for me it makes not much
> > sense. The constructor shouldn't check inheritance rules.
>
> Neither should other methods follow this. What if I want to add a
> non-default parameter to an overriden method?

Some test cases:

Should work #1, Bar::connect() adds an argument
-------------------------------------------------------------------
<?php
interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect($server) { }
}

class Bar extends Foo {
function connect($server, $port) { }
}
?>

Should work #2, Bar::connect() might contain something such as
parent::connect('foo.example.com');
-------------------------------------------------------------------
<?php
interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect($server) { }
}

class Bar extends Foo {
function connect() { }
}
?>

Should work #3, Bar::connect() might contain something such as
parent::connect($dsn->getHost());
-------------------------------------------------------------------
<?php
class DSN { }

interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect($server) { }
}

class Bar extends Foo {
function connect(DSN $dsn) { }
}
?>

Should work #4, Foo::connect() adds a default argument
-------------------------------------------------------------------
<?php
interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect($server, $port= 42) { }
}

class Bar extends Foo {
function connect($server, $port= 23) { }
}
?>

Should work #5, both interface and implementer have a default value
for the argument "server"
-------------------------------------------------------------------
<?php
interface Connector {
function connect($server= 'localhost');
}

class Foo implements Connector {
function connect($server= 'localhost') { }
}

class Bar extends Foo {
function connect($server= 'localhost', $port= 23) { }
}
?>

Should work #6, implementation adds a default value
-------------------------------------------------------------------
<?php
interface Connector {
function connect();
}

class Foo implements Connector {
function connect($server= 'localhost') { }
}

class Bar extends Foo {
function connect($server= 'localhost', $port= 23) { }
}
?>

Should NOT work #1, Foo doesn't fully implement connect()
-------------------------------------------------------------------
<?php
interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect() { }
}

class Bar extends Foo {
function connect($server, $port) { }
}
?>

Should NOT work #2, class / primitive clash on argument
-------------------------------------------------------------------
<?php
class DSN { }

interface Connector {
function connect(DSN $dsn);
}

class Foo implements Connector {
function connect($dsn) { }
}

class Bar extends Foo {
function connect($dsn) { }
}
?>

Should NOT work #3, Foo implements Connector::connect() incorrectly
-------------------------------------------------------------------
<?php
interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect($server, $port) { }
}

class Bar extends Foo {
function connect($server, $port= 23) { }
}
?>

Should NOT work #4, Foo implements Connector::connect() incorrectly
-------------------------------------------------------------------
<?php
interface Connector {
function connect();
}

class Foo implements Connector {
function connect($server) { }
}

class Bar extends Foo {
function connect($server, $port) { }
}
?>

I would simply refrain from calling
zend_do_perform_implementation_check() in inheritance.

- Timm

Marcus Börger

22 years ago

Hello Timm,

Thursday, February 26, 2004, 12:34:31 AM, you wrote:

> On Wed, 2004-02-25 at 23:52, Timm Friebe wrote:
>> On Wed, 2004-02-25 at 23:44, Marcus Boerger wrote:
>> > Hello Timm,
>> >
>> > i had the same expirience today too. And also for me it makes not much
>> > sense. The constructor shouldn't check inheritance rules.
>>
>> Neither should other methods follow this. What if I want to add a
>> non-default parameter to an overriden method?

> Some test cases:

> Should work #1, Bar::connect() adds an argument

No the sugnature is incompatible. An instance of Foo cannot be called
with Bar or Connector's connect() Signature. Hence Bar is not a Foo
or Connector.

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect($server) { }
>   }

>   class Bar extends Foo {
>     function connect($server, $port) { }
>   }

?>>

> Should work #2, Bar::connect() might contain something such as

Same asas above

>   parent::connect('foo.example.com');
> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect($server) { }
>   }

>   class Bar extends Foo {
>     function connect() { }
>   }

?>>

> Should work #3, Bar::connect() might contain something such as

Same as above. Foo::connect() can be called with any value
Bar::connect() not.

>   parent::connect($dsn->getHost());
> -------------------------------------------------------------------
> <?php
>   class DSN { }

>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect($server) { }
>   }

>   class Bar extends Foo {
>     function connect(DSN $dsn) { }
>   }

?>>

> Should work #4, Foo::connect() adds a default argument

Should work, since the calling convention from Connector is still
possible in Foo and Bar. Hence Foo::connect() IS-A Connector::connect()
and Bar::connect() IS-A Connector::connect().

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect($server, $port= 42) { }
>   }

>   class Bar extends Foo {
>     function connect($server, $port= 23) { }
>   }

?>>

> Should work #5, both interface and implementer have a default value
> for the argument "server"

Should work. Connector's calling convention is till valid in Bar.
Hence Foo::connect() IS-A Connector::connect() and Bar::connect()
IS-A Connector::connect().

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect($server= 'localhost');
>   }

>   class Foo implements Connector {
>     function connect($server= 'localhost') { }
>   }

>   class Bar extends Foo {
>     function connect($server= 'localhost', $port= 23) { }
>   }

?>>

> Should work #6, implementation adds a default value

Same as #5 only more complicated.

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect();
>   }

>   class Foo implements Connector {
>     function connect($server= 'localhost') { }
>   }

>   class Bar extends Foo {
>     function connect($server= 'localhost', $port= 23) { }
>   }

?>>

> Should NOT work #1, Foo doesn't fully implement connect()

Right shouldn't work.

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect() { }
>   }

>   class Bar extends Foo {
>     function connect($server, $port) { }
>   }

?>>

> Should NOT work #2, class / primitive clash on argument

Wrong. Should work. Foo::connect() IS-A Connector::connect() and
Bar::connect() IS-A Connector::connect(). It is no problem that a
derived clsses method accepts a greater value range or set of types.

> -------------------------------------------------------------------
> <?php
>   class DSN { }

>   interface Connector {
>     function connect(DSN $dsn);
>   }

>   class Foo implements Connector {
>     function connect($dsn) { }
>   }

>   class Bar extends Foo {
>     function connect($dsn) { }
>   }

?>>

> Should NOT work #3, Foo implements Connector::connect() incorrectly

Should work. Still Foo::connect() IS-A Connector::connect()
and Bar::connect() IS-A Connector::connect().

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect($server, $port) { }
>   }

>   class Bar extends Foo {
>     function connect($server, $port= 23) { }
>   }

?>>

> Should NOT work #4, Foo implements Connector::connect() incorrectly

Right, shouldn't work. Inheritance doesn't apply since the signatures
are incompatible.

> -------------------------------------------------------------------
> <?php
>   interface Connector {
>     function connect();
>   }

>   class Foo implements Connector {
>     function connect($server) { }
>   }

>   class Bar extends Foo {
>     function connect($server, $port) { }
>   }

?>>

> I would simply refrain from calling
> zend_do_perform_implementation_check() in inheritance.

> - Timm

--
Best regards,
Marcus                            mailto:helly@php.net

Timm Friebe

22 years ago

On Thu, 2004-02-26 at 01:38, Marcus Boerger wrote:

> Hello Timm,

[...]

> > Should work #1, Bar::connect() adds an argument
> No the sugnature is incompatible. An instance of Foo cannot be called
> with Bar or Connector's connect() Signature. Hence Bar is not a Foo
> or Connector.

Hrm, that's quite a (huge) BC break then. I know that adding a parameter
kind-of violates the contract between Bar and Connector, but - for an
instance - omit the interface idea and think about this:

class Error {
function __construct($message) { }
}

class SQLError extends Error {
function __construct($message, $sql) { }
}

This gives us an error. Or, not to restrict this to constructors:

class Printer {
function print() { }
}

class MultipleFormatCapablePrinter extends Printer {
function print($format) { }
}

This works just fine in PHP4, where, if I call SQLError's constructor
with one argument only, I'll simply get an E_WARNING.

You're changing this to a E_COMPILE_ERROR. Shouldn't some alarm bells
start ringing here?

With this new requirement, I'd have to make all additional parameters
optional in subclasses. This introduces more kludges:

class MultipleFormatCapablePrinter extends Printer {
function print($format= NULL) {
if (NULL === $format) {
throw new IllegalArgumentException('Format may not be NULL');
}
}
}

Now users can call this method (they could find out, e.g., via
Reflection_Method that format is an *optional* parameter - which means:
it can be ommitted, and if it is, it'll just be NULL - but it actually
can't, it's only declared with a default value because of a language
requirement) and won't even get an error if I don't catch it myself.

This doesn't come up in Java, as they have method overloading, so
MultipleFormatCapablePrinter::print() is actually a different method as
Printer::print() and you get away with it. And yes, PHP isn't Java.

[...]

> > Should NOT work #2, class / primitive clash on argument
> Wrong. Should work. Foo::connect() IS-A Connector::connect() and
> Bar::connect() IS-A Connector::connect(). It is no problem that a
> derived clsses method accepts a greater value range or set of types.

Well, this one:

> <?php
>   class DSN { }
>
>   interface Connector {
>     function connect(DSN $dsn);
>   }
>
>   class Foo implements Connector {
>     function connect($dsn) { }
>   }
>
>   class Bar extends Foo {
>     function connect($dsn) { }
>   }
> ?>

doesn't work right now, see zend_compile.c, lines 1737 - 1744. There's
even an inline comment (!) about it there:)

Even more weirdness:

thekid@friebes:~/devel/php/tests > cat inheritance.php
<?php
interface Connector {
function connect();
}

class Foo implements Connector {
function connect($server) { }
}
?>
thekid@friebes:~/devel/php/tests > php-dev inheritance.php
thekid@friebes:~/devel/php/tests > cat inheritance-b0rked.php
<?php
interface Connector {
function connect($server);
}

class Foo implements Connector {
function connect($server, $port) { }
}
?>
thekid@friebes:~/devel/php/tests > php-dev inheritance-b0rked.php

Fatal error: Declaration of Foo::connect() must be compatible with that
of Connector::connect() in
/usr/home/thekid/devel/php/tests/inheritance-b0rked.php on line 6

Huh? So having zero arguments in the interface and one in the
implementation is OK but having one in the interface and two in the
implementation is not?

- Timm

Marcus Börger

22 years ago

Hello Timm,

Hello Zeev, please have a look at this thread. Obviously you started
a huge BC break here. See my second comment on a possible way out.

Thursday, February 26, 2004, 2:13:48 AM, you wrote:

> On Thu, 2004-02-26 at 01:38, Marcus Boerger wrote:
>> Hello Timm,
> [...]
>> > Should work #1, Bar::connect() adds an argument
>> No the sugnature is incompatible. An instance of Foo cannot be called
>> with Bar or Connector's connect() Signature. Hence Bar is not a Foo
>> or Connector.

> Hrm, that's quite a (huge) BC break then. I know that adding a parameter
> kind-of violates the contract between Bar and Connector, but - for an
> instance - omit the interface idea and think about this:

>   class Error {
>     function __construct($message) { }
>   }

>   class SQLError extends Error {
>     function __construct($message, $sql) { }
>   }

> This gives us an error.

As i said i don't like this to be an error for constructors.

> Or, not to restrict this to constructors:

>   class Printer {
>     function print() { }
>   }

>   class MultipleFormatCapablePrinter extends Printer {
>     function print($format) { }
>   }

> This works just fine in PHP4, where, if I call SQLError's constructor
> with one argument only, I'll simply get an E_WARNING.

> You're changing this to a E_COMPILE_ERROR. Shouldn't some alarm bells
> start ringing here?

Yes! Maybe it would be good to apply the correct rules with E_COMPILE_ERROR
in cases where interfaces come into play and E_STRICT for compatibility mode
and non interfaces. Would that work for you?

> With this new requirement, I'd have to make all additional parameters
> optional in subclasses. This introduces more kludges:

>   class MultipleFormatCapablePrinter extends Printer {
>     function print($format= NULL) {
>       if (NULL === $format) {
>         throw new IllegalArgumentException('Format may not be NULL');
>       }
>     }
>   }

> Now users can call this method (they could find out, e.g., via
> Reflection_Method that format is an *optional* parameter - which means:
> it can be ommitted, and if it is, it'll just be NULL - but it actually
> can't, it's only declared with a default value because of a language
> requirement) and won't even get an error if I don't catch it myself.

> This doesn't come up in Java, as they have method overloading, so
> MultipleFormatCapablePrinter::print() is actually a different method as
> Printer::print() and you get away with it. And yes, PHP isn't Java.

> [...]
>> > Should NOT work #2, class / primitive clash on argument
>> Wrong. Should work. Foo::connect() IS-A Connector::connect() and
>> Bar::connect() IS-A Connector::connect(). It is no problem that a
>> derived clsses method accepts a greater value range or set of types.

> Well, this one:

>> <?php
>>   class DSN { }
>>
>>   interface Connector {
>>     function connect(DSN $dsn);
>>   }
>>
>>   class Foo implements Connector {
>>     function connect($dsn) { }
>>   }
>>
>>   class Bar extends Foo {
>>     function connect($dsn) { }
>>   }
>> ?>

> doesn't work right now, see zend_compile.c, lines 1737 - 1744. There's
> even an inline comment (!) about it there:)

> Even more weirdness:

> thekid@friebes:~/devel/php/tests > cat inheritance.php
> <?php
>   interface Connector {
>     function connect();
>   }

>   class Foo implements Connector {
>     function connect($server) { }
>   }

?>>

> thekid@friebes:~/devel/php/tests > php-dev inheritance.php
> thekid@friebes:~/devel/php/tests > cat inheritance-b0rked.php
> <?php
>   interface Connector {
>     function connect($server);
>   }

>   class Foo implements Connector {
>     function connect($server, $port) { }
>   }

?>>

> thekid@friebes:~/devel/php/tests > php-dev inheritance-b0rked.php

> Fatal error: Declaration of Foo::connect() must be compatible with that
> of Connector::connect() in
> /usr/home/thekid/devel/php/tests/inheritance-b0rked.php on line 6

> Huh? So having zero arguments in the interface and one in the
> implementation is OK but having one in the interface and two in the
> implementation is not?

> - Timm

--
Best regards,
Marcus                            mailto:helly@php.net

Timm Friebe

22 years ago

On Thu, 2004-02-26 at 02:38, Marcus Boerger wrote:

> Hello Timm,

[...]

> Yes! Maybe it would be good to apply the correct rules with E_COMPILE_ERROR
> in cases where interfaces come into play and E_STRICT for compatibility mode
> and non interfaces. Would that work for you?

Yupp, that's perfect.

- Timm

Timm Friebe

22 years ago

On Thu, 2004-02-26 at 02:13, Timm Friebe wrote:

> On Thu, 2004-02-26 at 01:38, Marcus Boerger wrote:
> > Hello Timm,
> [...]
> > > Should work #1, Bar::connect() adds an argument
> > No the sugnature is incompatible. An instance of Foo cannot be called
> > with Bar or Connector's connect() Signature. Hence Bar is not a Foo
> > or Connector.
>
> Hrm, that's quite a (huge) BC break then.

Maybe we'd all be happier with an E_STRICT warning for inheritance and
an E_COMPILE_ERROR for interfaces implementation.

- Timm

Derick Rethans

22 years ago

On Thu, 26 Feb 2004, Timm Friebe wrote:

> On Thu, 2004-02-26 at 01:38, Marcus Boerger wrote:
> > Hello Timm,
> [...]
> > > Should work #1, Bar::connect() adds an argument
> > No the sugnature is incompatible. An instance of Foo cannot be called
> > with Bar or Connector's connect() Signature. Hence Bar is not a Foo
> > or Connector.
>
> Hrm, that's quite a (huge) BC break then.

Right, I think we should not break BC in this instance.

Derick

Hans Lellelid

22 years ago

Timm Friebe wrote:

> On Wed, 2004-02-25 at 23:44, Marcus Boerger wrote:
>
>>Hello Timm,
>>
>>i had the same expirience today too. And also for me it makes not much
>>sense. The constructor shouldn't check inheritance rules.
>
>
> Neither should other methods follow this. What if I want to add a
> non-default parameter to an overriden method?
>

Yes, I think this is the same issue that I brought up earlier related to
interfaces.  I brought it up then as an inconsistency -- i.e. that you
couldn't override methods in interfaces & hence using interfaces was
limiting the OO inheritance that PHP supported when not using
interfaces.  In brief the issue was that if you extend a class that
implemented an interface the extending class (subclass) had to also
implement the parent class' interface and was therefore not allowed to
override methods (w/ incompatible signatures) -- and not allowed to
implement a different interface which itself might specify incompatible
signatures.

It seems now that PHP is no longer inconsistent, but it also seems that
it is impossible to override methods w/ incompatible signature.  Is that
a correct assessment?  This is a pretty big difference from PHP4, then.
Personally, I can live with it :) -- just want to make sure I
understand it correctly.

Thanks,
Hans

Andi Gutmans

22 years ago

At 18:54 25/02/2004 -0500, Hans Lellelid wrote:

>It seems now that PHP is no longer inconsistent, but it also seems that it
>is impossible to override methods w/ incompatible signature.  Is that a
>correct assessment?  This is a pretty big difference from PHP4,
>then.   Personally, I can live with it :) -- just want to make sure I
>understand it correctly.

You understand completely. It is incorrect to override methods w/
incompatible signature. We don't check this in compatibility mode.

Andi

Derick Rethans

22 years ago

On Thu, 26 Feb 2004, Andi Gutmans wrote:

> At 18:54 25/02/2004 -0500, Hans Lellelid wrote:
> >It seems now that PHP is no longer inconsistent, but it also seems that it
> >is impossible to override methods w/ incompatible signature.  Is that a
> >correct assessment?  This is a pretty big difference from PHP4,
> >then.   Personally, I can live with it :) -- just want to make sure I
> >understand it correctly.
>
> You understand completely. It is incorrect to override methods w/
> incompatible signature. We don't check this in compatibility mode.

IMO even outside compat mode it should just give an E_STRICT error type
message. This is a bit too much breakage without a really good reason
IMO.

Derick

Ferdinand Beyer

22 years ago

IMO we are trying to force a strict programming here that is
incompatible with PHP's loose character.

The following example for instance is very common in Java AVT
programming:

<?php

class Window {
function __construct($title)
{
}
}

class MyApplicationWindow extends Window
{
const VERSION = "1.1";

function __construct()
{
parent::__construct("My Application v." . self::VERSION");
}
}
?>

Furthermore with the new implementation we disallow "the PHP way
for overloaded methods" using a variable parameter count:

<?php

class Base
{
function doSomething(MyClass $obj)
{
}
}

class Extended extends Base
{
function doSomething()
{
$args = func_get_args();

switch (count($args)) {
case 0:
return parent::doSomething(new MyClass());

case 1:
if ($args[0] instanceof MyClass) {
return parent::doSomething($args[0]);
}
return parent::doSomething(new MyClass($args[0]));

default:
throw new IllegalArgumentException();
}
}
}

?>

--
Ferdinand Beyer
<fb@fbeyer.com>

Michael Walter

22 years ago

Ferdinand Beyer wrote:

> IMO we are trying to force a strict programming here that is
> incompatible with PHP's loose character.

Well, I don't get the point in relation to *constructors* at all.. I
mean, forcing the same signature for each constructor seems unreasonable
to me (_when explicitely called_, of couse).. there is no such thing as
a contract for constructor interfaces per default -- as you usually
don't call those explicitely (but when calling the parent's
constructor), that doesn't make too much (any) sense to me. Could
someone elaborate on the advantages of enforcing the same signature of
constructors (as long as the constructor doesn't get implicitely called,
of course)?

> The following example for instance is very common in Java AVT
> programming:
>
> [snip]
>

Yeah indeed, and extending/changing the constructor interface is simply
a natural thing when extending a class (by inheritance). Maybe I'm
completely missing the point, though, it might be too early for me.

> Furthermore with the new implementation we disallow "the PHP way
> for overloaded methods" using a variable parameter count:
>
> [...]

I'm less sure about normal functions. It probably makes sense to check
for matching signatures there (you can always handle optional additional
parameters by using default values/func_get_args(), [1]).

Cheers,
Michael

[1] Why is it that func_get_args() can't get used as a function
parameter directly, btw?

PS: I'm rather sorry to always bring in my personal views/opinions into
such discussions without actually contributing code to PHP5. In case
that is displeasing for [some|the core] people, I can of course stop
doing so ;) In the meanwhile, I hope that the percentage of posts
without braindead typos (as in the first IDIVL one) might enrich ongoing
discussions.

Hans Lellelid

22 years ago

Ferdinand Beyer wrote:

> IMO we are trying to force a strict programming here that is
> incompatible with PHP's loose character.
>

Yeah, I absolutely agree with that.  The more I think about it the more
it also seems like this change is going to break a *lot* of PHP code out
there.  Especially the example that Timm and Ferdinand give w/ constructors.

I'm already frustrated that in my JDBC-like abstraction layer my
PreparedStatement class cannot extend Statement, because
PreparedStatement has an executeQuery() method that does not take any
parameters, while the more generic Statement::executeQuery($sql) needs
the query passed to it.  In every other way PrepareStatement is a
subclass.  I guess basically I think that if PHP is not going to support
overloading, then it needs to allow for a looser interpretation of the
isA contract.  In that case I'm also using interfaces, and I realize
that interfaces require the inheritance to be a bit more strict -- and
I'm at peace with that :) (though I really think interfaces should be
allowed to override too, just to be consistent in PHP).

This especially bothers me, I suppose, because I've been doing a lot of
work creating PHP5 versions of a few Java apps (Torque, Ant).  This
change will break a great deal of my code, and I can only imagine that
it will break many other existing PHP4 classes out there.  I don't want
to have to turn compatibility mode on for code that I wrote for PHP5 (!)

> The following example for instance is very common in Java AVT
> programming:
>
> <?php
>
> class Window {
>     function __construct($title)
>     {
>     }
> }
>
> class MyApplicationWindow extends Window
> {
>     const VERSION = "1.1";
>
>     function __construct()
>     {
>         parent::__construct("My Application v." . self::VERSION");
>     }
> }
> ?>
>

Yeah, I can think of a million examples where I've done something like
that.  (Like my PreparedStatement / Statement example.)

Hans

Zeev Suraski

22 years ago

At 07:19 26/02/2004, Hans Lellelid wrote:

>Ferdinand Beyer wrote:
>
>>IMO we are trying to force a strict programming here that is incompatible
>>with PHP's loose character.
>
>Yeah, I absolutely agree with that.  The more I think about it the more it
>also seems like this change is going to break a *lot* of PHP code out
>there.  Especially the example that Timm and Ferdinand give w/ constructors.
>
>I'm already frustrated that in my JDBC-like abstraction layer my
>PreparedStatement class cannot extend Statement, because PreparedStatement
>has an executeQuery() method that does not take any parameters, while the
>more generic Statement::executeQuery($sql) needs the query passed to
>it.  In every other way PrepareStatement is a subclass.  I guess basically
>I think that if PHP is not going to support overloading, then it needs to
>allow for a looser interpretation of the isA contract.  In that case I'm
>also using interfaces, and I realize that interfaces require the
>inheritance to be a bit more strict -- and I'm at peace with that :)
>(though I really think interfaces should be allowed to override too, just
>to be consistent in PHP).
>
>This especially bothers me, I suppose, because I've been doing a lot of
>work creating PHP5 versions of a few Java apps (Torque, Ant).  This change
>will break a great deal of my code, and I can only imagine that it will
>break many other existing PHP4 classes out there.  I don't want to have to
>turn compatibility mode on for code that I wrote for PHP5 (!)

If you take into account that [a] in PHP, you cannot have more than one
signature for a method in a given class, and you take into account the fact
that [b] your overriding method must be able to satisfy the same interface
as the method its overriding (because it may be called from a context that
was written to work with its parent method), then it all makes a lot of
sense.  You solution may be to declare PreparedStatement::executeQuery()
with $sql=null as an argument that can be omitted, or even forces it to be
omitted (if it's not null, display an error).  It sounds to me as if you
haven't taken [b] into account, and I guess there are many people that fall
in that pitfall.  But that's exactly what this error message comes to solve
- if you don't take it into account, you are, in one way or another,
developing error-prone code.

Constructors will probably have to be dealt with differently.

Zeev

George Schlossnagle

22 years ago

On Feb 26, 2004, at 11:49 PM, Zeev Suraski wrote:

> At 07:19 26/02/2004, Hans Lellelid wrote:
>> Ferdinand Beyer wrote:
>>
>>> IMO we are trying to force a strict programming here that is
>>> incompatible with PHP's loose character.
>>
>> Yeah, I absolutely agree with that.  The more I think about it the
>> more it also seems like this change is going to break a *lot* of PHP
>> code out there.  Especially the example that Timm and Ferdinand give
>> w/ constructors.
>>
>> I'm already frustrated that in my JDBC-like abstraction layer my
>> PreparedStatement class cannot extend Statement, because
>> PreparedStatement has an executeQuery() method that does not take any
>> parameters, while the more generic Statement::executeQuery($sql)
>> needs the query passed to it.  In every other way PrepareStatement is
>> a subclass.  I guess basically I think that if PHP is not going to
>> support overloading, then it needs to allow for a looser
>> interpretation of the isA contract.  In that case I'm also using
>> interfaces, and I realize that interfaces require the inheritance to
>> be a bit more strict -- and I'm at peace with that :) (though I
>> really think interfaces should be allowed to override too, just to be
>> consistent in PHP).
>>
>> This especially bothers me, I suppose, because I've been doing a lot
>> of work creating PHP5 versions of a few Java apps (Torque, Ant).
>> This change will break a great deal of my code, and I can only
>> imagine that it will break many other existing PHP4 classes out
>> there.  I don't want to have to turn compatibility mode on for code
>> that I wrote for PHP5 (!)
>
> If you take into account that [a] in PHP, you cannot have more than
> one signature for a method in a given class, and you take into account
> the fact that [b] your overriding method must be able to satisfy the
> same interface as the method its overriding (because it may be called
> from a context that was written to work with its parent method), then
> it all makes a lot of sense.  You solution may be to declare
> PreparedStatement::executeQuery() with $sql=null as an argument that
> can be omitted, or even forces it to be omitted (if it's not null,
> display an error).  It sounds to me as if you haven't taken [b] into
> account, and I guess there are many people that fall in that pitfall.
> But that's exactly what this error message comes to solve - if you
> don't take it into account, you are, in one way or another, developing
> error-prone code.

This is an enormously huge bc break.  Error-prone or not, I would wager
that 95%+ of all php4 OO code exploits the ability to redefine the
signature on inherited methods.  It also seems really against the
spirit of PHP to enforce this sort of thing.

George

Adam Maccabee Trachtenberg

22 years ago

On Fri, 27 Feb 2004, George Schlossnagle wrote:

> This is an enormously huge bc break.  Error-prone or not, I would wager
> that 95%+ of all php4 OO code exploits the ability to redefine the
> signature on inherited methods.

So far, I've identified that this breaks more than one important PEAR
package: XML_Parser, HTML_Quickform, as well as the PEAR package
manager (which has the side effect of breaking the PHP install
process).

Additionally, it borks ADODB. And I can't test Smarty and PEAR::SOAP
because right now they're dumping core. :)

> It also seems really against the spirit of PHP to enforce this sort
> of thing.

I strongly agree.

-adam

--
adam@trachtenberg.com
author of o'reilly's php cookbook
avoid the holiday rush, buy your copy today!

Ferdinand Beyer

22 years ago

On 27 Feb 2004 at 11:12, Andi Gutmans wrote:

> Comments? (Please try to keep them short :)

I think we should just drop the signature check. Interfaces may
include a signature for better readability but the engine should ignore
them. PHP4 class trees work fine even without strict inheritance
checks.

Because of PHP's loose character, we cannot force the user to use
the strict signature anyway. And since we have no overloading
support, but var_args, strict checks make not much sense to me.

--
Ferdinand Beyer
<fb@fbeyer.com>

Patrick Schnorbus

22 years ago

On Friday 27 February 2004 10:41, Ferdinand Beyer wrote:

> On 27 Feb 2004 at 11:12, Andi Gutmans wrote:
> > Comments? (Please try to keep them short :)
>
> I think we should just drop the signature check. Interfaces may
> include a signature for better readability but the engine should ignore
> them. PHP4 class trees work fine even without strict inheritance
> checks.

Mhm. Does a signature make sense if the engine ignores it anyway?
For better readability one can use phpdoc/doxygen like comment blocks.

> Because of PHP's loose character, we cannot force the user to use
> the strict signature anyway. And since we have no overloading
> support, but var_args, strict checks make not much sense to me.

Agree.

> --
> Ferdinand Beyer
> <fb@fbeyer.com>

cheers,
Pat

Ferdinand Beyer

22 years ago

On 27 Feb 2004 at 10:58, Patrick Schnorbus wrote:

> Mhm. Does a signature make sense if the engine ignores it

anyway?

> For better readability one can use phpdoc/doxygen like comment

blocks.

That's right. But I don't see a reason why we should disallow to use a
signature in an interface declaration even if it is ignored.

--
Ferdinand Beyer
<fb@fbeyer.com>

Andi Gutmans

22 years ago

Hey,

I'd like to come to some conclusion about the latest changes which break BC
(trying to keep it short because some people here wrote long essays and it
took me too much time to catch up :)  :
a) I agree that it doesn't make much sense for constructors, because one
always creates concrete classes.
b) I think we pretty much all agree that the change is correct but it might
be a bit too aggressive for certain people (such as people using var_args)
and definitely too aggressive for existing code.
c) Concerning the differentiating between interfaces and inheritance, in
general, I believe these are exactly the same thing and should behave
equally. However, we do have a luxury of doing whatever we want when it
comes to interfaces and abstract classes because they didn't exist in PHP
4, though treating them differently than inheritance will be a bit
inconsistent.
d) Last problem is that using E_STRICT is not a good idea because we find
this problem at compile-time and the error_reporting might not be set yet.

What I suggest is the following:
a) Don't check signature for constructors.
b) By default, don't check signature for inheritance *if* we're not
inheriting from an abstract class. If it is an abstract class we should
check the signature because it's a PHP 5 feature.
c) Add a new INI option (zend.strict_inheritance_checks) which does check
signature for methods (except for constructor). I don't like new INI
options but I don't think there's any way out.

Comments? (Please try to keep them short :)

Andi

Stanislav Malyshev

22 years ago

AG>> c) Add a new INI option (zend.strict_inheritance_checks) which
AG>> does check signature for methods (except for constructor). I
AG>> don't like new INI options but I don't think there's any way
AG>> out.

There's a dilemma about this option. On one hand, it should be on by
default. Otherwise, people would code for default configuration, it would
work for default, but misteriously fail on some setups. If it's default,
one would have to code so that signatures are OK and it would work on both
default and non-default. But this way it makes the option a bit of
pointless except as a way to preserve old code running. On the other hand,
most big shops would have to set it to off when they move to PHP5
otherwise everybody would cry "PHP5 broke my code". Tricky.

--
Stanislav Malyshev, Zend Products Engineer
stas@zend.com  http://www.zend.com/ +972-3-6139665 ext.109

Stephane Drouard

22 years ago

== Quote from Andi Gutmans (andi@zend.com)'s article

> a) Don't check signature for constructors.

Constructors are definitely not virtual methods. So no reason to check signature.
Moreover, this is a non sense to declare constructors in interfaces or abstract constructors in abstract classes.

> b) By default, don't check signature for inheritance *if* we're not
> inheriting from an abstract class. If it is an abstract class we should
> check the signature because it's a PHP 5 feature.

I assume "abstract class" includes interfaces.

> c) Add a new INI option (zend.strict_inheritance_checks) which does check
> signature for methods (except for constructor). I don't like new INI
> options but I don't think there's any way out.

Because we could discover some other compatibility problems when moving to PHP5, what about an INI option using flags, same as error_reporting.

For exemple zend.php5_compat and one of it's flag NO_STRICT_INHERITANCE.

Another flag could be SET_CLONE to have "$obj2 = $obj;" or "foo($obj);" compatible with PHP4 (a clone rather than a reference). Indeed I think there will be some side effects when executing some PHP4 code under PHP5.

Regards,
Stephane

Eric Daspet

22 years ago

Andi Gutmans wrote:

> b) By default, don't check signature for inheritance *if* we're not
> inheriting from an abstract class. If it is an abstract class we should
> check the signature because it's a PHP 5 feature.

Do you mean :

- check the signature only for methods which were in the abstract class
(I hope it is this solution)
or
- check the signature for all methods if a class is inheriting from an
abstract (assuming that if it uses an abstract somewhere then we are not
in compatibility and we can use the strict checks for all the class
methods except constructor)


Exemple of what I mean is below :

abstract class myAbstract {
public function fromAbstract() ;
}
interface myInterface {
public function fromInterface() ;
}

class myClass extends myAbstract implements myInterface {
public function fromAbstract() {
// ... ** If I understand correctly we check signature for this one
}
public function fromInterface() {
// ... ** If I understand correctly we check signature for this one
}
public function otherMethod() {
// ...
}
}

class myInheritedCLass extends myClass {
public function otherMethod() {
// ... ** Do web check signature of this one ?
}
public function fromAbstract() {
// ... ** If I understand correctly we check signature for this one
}
public function fromInterface() {
// ... ** If I understand correctly we check signature for this one
}
}

> c) Add a new INI option (zend.strict_inheritance_checks)

Will it be possible to override it at runtime with ini_set() ? (even if
it will not be necessary if it is disable per default)

--
Eric

Patrick Schnorbus

22 years ago

On Friday 27 February 2004 10:12, Andi Gutmans wrote:

> What I suggest is the following:
> a) Don't check signature for constructors.

Okie ;)

> b) By default, don't check signature for inheritance *if* we're not
> inheriting from an abstract class.
> If it is an abstract class we should
> check the signature because it's a PHP 5 feature.

Yeah, for abstract classes the checks are fine and there won't be any
compliance problems.

> c) Add a new INI option (zend.strict_inheritance_checks) which does check
> signature for methods (except for constructor). I don't like new INI
> options but I don't think there's any way out.

Me neither, worse luck. But we should name the option something like
zend.legacy_compat for other potential compliance problems like cloning. (At
the time of writing i see Stephane Drouard made a similalar suggestion).
This option needs to be set by default, but we should make clear that it won't
be default for ever.

>
> Comments? (Please try to keep them short :)
>
> Andi

cheers,
Pat

Derick Rethans

22 years ago

On Fri, 27 Feb 2004, Andi Gutmans wrote:

> d) Last problem is that using E_STRICT is not a good idea because we find
> this problem at compile-time and the error_reporting might not be set yet.

The INI system is 'booted' before the script is compiled, so I doubt
that this is a problem.

> What I suggest is the following:
> a) Don't check signature for constructors.
> b) By default, don't check signature for inheritance *if* we're not
> inheriting from an abstract class. If it is an abstract class we should
> check the signature because it's a PHP 5 feature.

ack and ack

> c) Add a new INI option (zend.strict_inheritance_checks) which does check
> signature for methods (except for constructor). I don't like new INI
> options but I don't think there's any way out.

I'm against adding another INI setting. I don't see why we need to force
this OO purist thing on all long time users at all. What is wrong by
showing an e_notice/e_warning for it in non-compat mode, and e_strict in
compat mode (or just e_strict in both cases)? Then PHP still tells you
that you do something wrong, but it won't break any code.

Derick

Stig S. Bakken

22 years ago

On Fri, 2004-02-27 at 14:13, Derick Rethans wrote:

> On Fri, 27 Feb 2004, Andi Gutmans wrote:
>
> > d) Last problem is that using E_STRICT is not a good idea because we find
> > this problem at compile-time and the error_reporting might not be set yet.
>
> The INI system is 'booted' before the script is compiled, so I doubt
> that this is a problem.
>
> > What I suggest is the following:
> > a) Don't check signature for constructors.
> > b) By default, don't check signature for inheritance *if* we're not
> > inheriting from an abstract class. If it is an abstract class we should
> > check the signature because it's a PHP 5 feature.
>
> ack and ack
>
> > c) Add a new INI option (zend.strict_inheritance_checks) which does check
> > signature for methods (except for constructor). I don't like new INI
> > options but I don't think there's any way out.
>
> I'm against adding another INI setting. I don't see why we need to force
> this OO purist thing on all long time users at all. What is wrong by
> showing an e_notice/e_warning for it in non-compat mode, and e_strict in
> compat mode (or just e_strict in both cases)? Then PHP still tells you
> that you do something wrong, but it won't break any code.

What is worse is that many PHP 4 APIs need to be completely redesigned
for PHP 5, not only the guts of the class.

IMHO method overloading the the only clean way out of this.

- Stig

George Schlossnagle

22 years ago

On Feb 27, 2004, at 4:12 AM, Andi Gutmans wrote:

> Hey,
>
> I'd like to come to some conclusion about the latest changes which
> break BC (trying to keep it short because some people here wrote long
> essays and it took me too much time to catch up :)  :
> a) I agree that it doesn't make much sense for constructors, because
> one always creates concrete classes.

To play devil's advocate, If I'm creating objects through a factory,
the unified constructor signature is very helpful.

> d) Last problem is that using E_STRICT is not a good idea because we
> find this problem at compile-time and the error_reporting might not be
> set yet.

PHP is plagued with this problem though (if you consider it a problem,
I think it's a feature).  This is not the only warning in PHP that is
suppressed at compile time if your error settings aren't set correctly.

>
> What I suggest is the following:
> a) Don't check signature for constructors.

I like that.

> b) By default, don't check signature for inheritance *if* we're not
> inheriting from an abstract class. If it is an abstract class we
> should check the signature because it's a PHP 5 feature.

I'm iffy on that, but it's certainly better than the current situation.

> c) Add a new INI option (zend.strict_inheritance_checks) which does
> check signature for methods (except for constructor). I don't like new
> INI options but I don't think there's any way out.

I prefer E_STRICT warnings unconditionally.  The new ini suffers from
all the issues ini settings do (hinders portable code).

George

Michael Walter

22 years ago

George Schlossnagle wrote:

>
> On Feb 27, 2004, at 4:12 AM, Andi Gutmans wrote:
>
>> Hey,
>>
>> I'd like to come to some conclusion about the latest changes which
>> break BC (trying to keep it short because some people here wrote long
>> essays and it took me too much time to catch up :)  :
>> a) I agree that it doesn't make much sense for constructors, because
>> one always creates concrete classes.
>
>
> To play devil's advocate, If I'm creating objects through a factory, the
> unified constructor signature is very helpful.

So put a constructor declaration into the interface and have that one
validated.

Cheers,
Michael

Andi Gutmans

22 years ago

At 14:13 27/02/2004 +0100, Derick Rethans wrote:

>On Fri, 27 Feb 2004, Andi Gutmans wrote:
>
> > d) Last problem is that using E_STRICT is not a good idea because we find
> > this problem at compile-time and the error_reporting might not be set yet.
>
>The INI system is 'booted' before the script is compiled, so I doubt
>that this is a problem.

Yeah but people are used to setting the error_reporting from prepend's or
at the beginning of the script. But you know what, I guess we can live with
this because I see a few E_WARNINGS at the compile stage. So I guess we
could go with E_STRICT for people who want to get the warning (and the
script will still run).
The question is if we should ignore constructors from this check. I think
we should but one person here thought we shouldn't.

> > What I suggest is the following:
> > a) Don't check signature for constructors.
> > b) By default, don't check signature for inheritance *if* we're not
> > inheriting from an abstract class. If it is an abstract class we should
> > check the signature because it's a PHP 5 feature.
>
>ack and ack

Is everyone OK with peforming this check for abstract classes and
interfaces? Or do you think it should only be checked for E_STRICT (before
our change it was checked! We only changed inheritance).

> > c) Add a new INI option (zend.strict_inheritance_checks) which does check
> > signature for methods (except for constructor). I don't like new INI
> > options but I don't think there's any way out.
>
>I'm against adding another INI setting. I don't see why we need to force
>this OO purist thing on all long time users at all. What is wrong by
>showing an e_notice/e_warning for it in non-compat mode, and e_strict in
>compat mode (or just e_strict in both cases)? Then PHP still tells you
>that you do something wrong, but it won't break any code.

That could work too. So in zend.ze2_compatibility_mode we'd do an E_STRICT
and otherwise an E_WARNING? I think it's even OK if we just go with
E_STRICT in all cases.

Andi

George Schlossnagle

22 years ago

On Feb 27, 2004, at 10:05 AM, Andi Gutmans wrote:

>
> Yeah but people are used to setting the error_reporting from prepend's
> or at the beginning of the script. But you know what, I guess we can
> live with this because I see a few E_WARNINGS at the compile stage. So
> I guess we could go with E_STRICT for people who want to get the
> warning (and the script will still run).
> The question is if we should ignore constructors from this check. I
> think we should but one person here thought we shouldn't.

Just to be clear - I'm not that person, right?

>
>> > What I suggest is the following:
>> > a) Don't check signature for constructors.
>> > b) By default, don't check signature for inheritance *if* we're not
>> > inheriting from an abstract class. If it is an abstract class we
>> should
>> > check the signature because it's a PHP 5 feature.
>>
>> ack and ack
>
> Is everyone OK with peforming this check for abstract classes and
> interfaces? Or do you think it should only be checked for E_STRICT
> (before our change it was checked! We only changed inheritance).
>
>> > c) Add a new INI option (zend.strict_inheritance_checks) which does
>> check
>> > signature for methods (except for constructor). I don't like new INI
>> > options but I don't think there's any way out.
>>
>> I'm against adding another INI setting. I don't see why we need to
>> force
>> this OO purist thing on all long time users at all. What is wrong by
>> showing an e_notice/e_warning for it in non-compat mode, and e_strict
>> in
>> compat mode (or just e_strict in both cases)? Then PHP still tells you
>> that you do something wrong, but it won't break any code.
>
> That could work too. So in zend.ze2_compatibility_mode we'd do an
> E_STRICT and otherwise an E_WARNING? I think it's even OK if we just
> go with E_STRICT in all cases.

I'd prefer E_STRICT/E_STRICT or E_STRICT/E_NOTICE, but I can live with
this as well.

George

Derick Rethans

22 years ago

On Fri, 27 Feb 2004, Andi Gutmans wrote:

> At 14:13 27/02/2004 +0100, Derick Rethans wrote:
> >On Fri, 27 Feb 2004, Andi Gutmans wrote:
> >
> > > d) Last problem is that using E_STRICT is not a good idea because we find
> > > this problem at compile-time and the error_reporting might not be set yet.
> >
> >The INI system is 'booted' before the script is compiled, so I doubt
> >that this is a problem.
>
> Yeah but people are used to setting the error_reporting from prepend's or
> at the beginning of the script. But you know what, I guess we can live with
> this because I see a few E_WARNINGS at the compile stage. So I guess we
> could go with E_STRICT for people who want to get the warning (and the
> script will still run).
> The question is if we should ignore constructors from this check. I think
> we should but one person here thought we shouldn't.

I think we should too.

> That could work too. So in zend.ze2_compatibility_mode we'd do an E_STRICT
> and otherwise an E_WARNING? I think it's even OK if we just go with
> E_STRICT in all cases.

yeah, that sounds better to me too.

Derick

Hans Lellelid

22 years ago

Hi -

Andi Gutmans wrote:

>
> The question is if we should ignore constructors from this check. I
> think we should but one person here thought we shouldn't.

I think constructors should be ignored.

>
> Is everyone OK with peforming this check for abstract classes and
> interfaces? Or do you think it should only be checked for E_STRICT
> (before our change it was checked! We only changed inheritance).

I tend to think that for consistency interfaces should be allowed to
override method signatures (i.e. when extending an interface you can
change it), but agree that this is technically wrong/bad behavior.

Just a though -- would it make sense to have another keywords / method
that does a strict isA check?  -- i.e. if ($obj strictinstanceof
Statement).  This would allow developers to make those
signature-conforming assumptions that they cannot make in PHP now.

>> > c) Add a new INI option (zend.strict_inheritance_checks) which does
>> check
>> > signature for methods (except for constructor). I don't like new INI
>> > options but I don't think there's any way out.
>>
>> I'm against adding another INI setting. I don't see why we need to force
>> this OO purist thing on all long time users at all. What is wrong by
>> showing an e_notice/e_warning for it in non-compat mode, and e_strict in
>> compat mode (or just e_strict in both cases)? Then PHP still tells you
>> that you do something wrong, but it won't break any code.
>
>
> That could work too. So in zend.ze2_compatibility_mode we'd do an
> E_STRICT and otherwise an E_WARNING? I think it's even OK if we just go
> with E_STRICT in all cases.

I'd prefer E_STRICT always, if anything, since I want to be able to
design PHP5 code that takes advantage of loose inheritance checking w/o
requiring users to enable compatibilty mode.

Hans

Andi Gutmans

22 years ago

At 10:25 27/02/2004 -0500, Hans Lellelid wrote:

>Hi -
>
>Andi Gutmans wrote:
>>The question is if we should ignore constructors from this check. I think
>>we should but one person here thought we shouldn't.
>
>I think constructors should be ignored.

OK

>>Is everyone OK with peforming this check for abstract classes and
>>interfaces? Or do you think it should only be checked for E_STRICT
>>(before our change it was checked! We only changed inheritance).
>
>I tend to think that for consistency interfaces should be allowed to
>override method signatures (i.e. when extending an interface you can
>change it), but agree that this is technically wrong/bad behavior.
>
>Just a though -- would it make sense to have another keywords / method
>that does a strict isA check?  -- i.e. if ($obj strictinstanceof
>Statement).  This would allow developers to make those
>signature-conforming assumptions that they cannot make in PHP now.

No. I wouldn't want to add yet another keyword. I suggest that whoever
wants to be pure uses E_STRICT.

>>> > c) Add a new INI option (zend.strict_inheritance_checks) which does check
>>> > signature for methods (except for constructor). I don't like new INI
>>> > options but I don't think there's any way out.
>>>
>>>I'm against adding another INI setting. I don't see why we need to force
>>>this OO purist thing on all long time users at all. What is wrong by
>>>showing an e_notice/e_warning for it in non-compat mode, and e_strict in
>>>compat mode (or just e_strict in both cases)? Then PHP still tells you
>>>that you do something wrong, but it won't break any code.
>>
>>That could work too. So in zend.ze2_compatibility_mode we'd do an
>>E_STRICT and otherwise an E_WARNING? I think it's even OK if we just go
>>with E_STRICT in all cases.
>
>I'd prefer E_STRICT always, if anything, since I want to be able to design
>PHP5 code that takes advantage of loose inheritance checking w/o requiring
>users to enable compatibilty mode.

Okay.

Andi

Andrei Zmievski

22 years ago

On Fri, 27 Feb 2004, Andi Gutmans wrote:

> What I suggest is the following:
> a) Don't check signature for constructors.
> b) By default, don't check signature for inheritance *if* we're not
> inheriting from an abstract class. If it is an abstract class we should
> check the signature because it's a PHP 5 feature.
> c) Add a new INI option (zend.strict_inheritance_checks) which does check
> signature for methods (except for constructor). I don't like new INI
> options but I don't think there's any way out.

Sounds good to me.

- Andrei

Hans Lellelid

22 years ago

Hi Zeev -

Thanks for the response.

Zeev Suraski wrote:

> If you take into account that [a] in PHP, you cannot have more than one
> signature for a method in a given class, and you take into account the
> fact that [b] your overriding method must be able to satisfy the same
> interface as the method its overriding (because it may be called from a
> context that was written to work with its parent method), then it all
> makes a lot of sense.

Yes, this does make sense when interfaces are involved (as they are in
my PreparedStatement example).  It's more that it's frustrating because
the way around this is kludgy, and I'm not sure what's worse:  a "loose"
OO inheritance model or kludgy code as the solution to workaround a
strict model.

I agree with others that E_STRICT error for method signature change
would be fine.  I think to raise a fatal error would break way too much
PHP4 (and PHP5) code.

You solution may be to declare

> PreparedStatement::executeQuery() with $sql=null as an argument that can
> be omitted, or even forces it to be omitted (if it's not null, display
> an error).

Yeah, this feels like the kind of hack that I would like to avoid.  If
you always throw an Exception when $sql is set in PreparedStatement,
then in *practice* this is really no different from breaking the isA
contract since the method cannot be used with same signature as parent
method.  In my case I just chose not to inherit PreparedStatement from
Statement; that way I can change the method signatures without problems
arising from the inherited interfaces.  Not ideal, but it works.

>
> Constructors will probably have to be dealt with differently.

Good -- whew :)

Thanks-
Hans

Greg Beaver

22 years ago

Zeev Suraski wrote:

> If you take into account that [a] in PHP, you cannot have more than one
> signature for a method in a given class, and you take into account the
> fact that [b] your overriding method must be able to satisfy the same
> interface as the method its overriding

The real solution is not to assume that the inheritance contract is in
place, but to make it explicit.  Final is used to prevent inheritance
altogether, why not use the implements keyword as an explicit
inheritance contract, and otherwise assume there is none.  This
preserves BC 100%

No need to futz with php.ini, this would be a mistake.

<?php
class first {
function method($a)
{
}
}

class second extends first {
function method() // this is fine
{
parent::method('something');
}
}

class third implements someinterface {
function method($a)
{
}
}

class fourth extends third {
function method() // compile error
{
parent::method('something');
}
}
?>

Greg

Andi Gutmans

22 years ago

At 23:52 25/02/2004 +0100, Timm Friebe wrote:

>On Wed, 2004-02-25 at 23:44, Marcus Boerger wrote:
> > Hello Timm,
> >
> > i had the same expirience today too. And also for me it makes not much
> > sense. The constructor shouldn't check inheritance rules.
>
>Neither should other methods follow this. What if I want to add a
>non-default parameter to an overriden method?

Uhmmm if you do that then Bar isA Foo will not be correct.

><?php
>   class Foo {
>     function connect($server) {
>     }
>   }
>
>   class Bar extends Foo {
>     function connect($server, $port) {
>     }
>   }
>?>
>
>I see where the problem comes from:
>
>zend_do_perform_implementation_check() is called from
>do_inherit_method_check() (both in zend_compile.c) which in turn is
>called for inheritance *and* for interfaces. The behaviour is fully
>desirable when implementing interfaces but not for regular inheritance.

Sure it is. Regular inheritance has to follow the same isA rules as
interfaces. Inheritance is exactly the same thing except that it also has
code and private/protected methods/members whereas interfaces don't.

Andi

Andrey Hristov

22 years ago

Marcus Boerger wrote:

> Hello Timm,
>
> i had the same expirience today too. And also for me it makes not much
> sense. The constructor shouldn't check inheritance rules. And as a
> consequence maybe interfaces shouldn't allow constructors.
>

Does inheritance include visibility rules? ;)

Andrey

Marcus Börger

22 years ago

Hello Andrey,

Thursday, February 26, 2004, 1:08:19 PM, you wrote:

> Marcus Boerger wrote:
>> Hello Timm,
>>
>> i had the same expirience today too. And also for me it makes not much
>> sense. The constructor shouldn't check inheritance rules. And as a
>> consequence maybe interfaces shouldn't allow constructors.
>>
> Does inheritance include visibility rules? ;)

You can only increase wivibility (pretected-> public) in a strict IS-A
modell as PHP uses.

--
Best regards,
Marcus                            mailto:helly@php.net

Andi Gutmans

22 years ago

At 23:36 25/02/2004 +0100, Timm Friebe wrote:

>thekid@friebes:~/devel/php/tests > cat inheritance.php
><?php
>   class Foo {
>     function __construct($foo) {
>     }
>   }
>
>   class Bar extends Foo {
>     function __construct($foo, $bar) {
>       // Add = NULL after $bar to make it work
>     }
>   }
>?>
>thekid@friebes:~/devel/php/tests > php-dev inheritance.php
>
>Fatal error: Declaration of Bar::__construct() must be compatible with
>that of Foo::__construct() in
>/usr/home/thekid/devel/php/tests/inheritance.php on line 10
>
>Is this really necessary?

Guys,

You are breaking the isA relationship. We fixed this so that from now on,
people will not make such mistakes anymore (I think it's the right way to
go, so that we don't leave broken functionality around).
You can enable compatibility mode to make this work or specify a default
value (as you saw) for $bar so that you are keeping the interface of the
base class.

Andi

Timm Friebe

22 years ago

On Thu, 2004-02-26 at 07:49, Andi Gutmans wrote:

> At 23:36 25/02/2004 +0100, Timm Friebe wrote:

[...]

> Guys,
>
> You are breaking the isA relationship. We fixed this so that from now on,
> people will not make such mistakes anymore (I think it's the right way to
> go, so that we don't leave broken functionality around).

We do so ourselves:

class Reflection_Function implements Reflector {
public __construct(string name)
public mixed invoke(mixed* args)
}

class Reflection_Method extends Reflection_Function {
public __construct(mixed class, string name)
public mixed invoke(stdclass object, mixed* args)
}

But, of course, the Engine checks are not imposed on internal classes.

I'm just saying this is quite a BC break, whether it follows strict OOP
rules or not.

- Timm

Andi Gutmans

22 years ago

At 08:03 26/02/2004 +0100, Timm Friebe wrote:

>On Thu, 2004-02-26 at 07:49, Andi Gutmans wrote:
> > At 23:36 25/02/2004 +0100, Timm Friebe wrote:
>[...]
> > Guys,
> >
> > You are breaking the isA relationship. We fixed this so that from now on,
> > people will not make such mistakes anymore (I think it's the right way to
> > go, so that we don't leave broken functionality around).
>
>We do so ourselves:
>
>class Reflection_Function implements Reflector {
>   public __construct(string name)
>   public mixed invoke(mixed* args)
>}
>
>class Reflection_Method extends Reflection_Function {
>   public __construct(mixed class, string name)
>   public mixed invoke(stdclass object, mixed* args)
>}
>
>But, of course, the Engine checks are not imposed on internal classes.

It does if the internal extension supplies ARG_INFO.

>I'm just saying this is quite a BC break, whether it follows strict OOP
>rules or not.

Yes, I understand that and I understand why you think it's a problem. The
problem with E_STRICT is that people might think they can turn it on with
error_reporting() but for this behavior to change (as it's compile time) it
needs to be done via php.ini or htaccess.
So I guess if you all prefer this as optional, we'd have to either add yet
another INI directive or put it in E_STRICT (which as I mentioned will
confuse people who are used to being able to change E_* from their scripts).

Andi

Andi

Jeff Moore

22 years ago

On Feb 26, 2004, at 1:49 AM, Andi Gutmans wrote:

> You are breaking the isA relationship. We fixed this so that from now
> on, people will not make such mistakes anymore (I think it's the right
> way to go, so that we don't leave broken functionality around).
> You can enable compatibility mode to make this work or specify a
> default value (as you saw) for $bar so that you are keeping the
> interface of the base class.

First Interfaces.  Interfaces are a purely a type checking construct.
There is nothing you can do with interfaces in PHP 5 that you can't
already do with classes in PHP 4 except type checking.  So, it makes
sense that implementing an interface obligates you to some method
signature type checking. Implementing an interface means this class
supports this contract.

interface BaseInterface1 {
function example($value = NULL);
}

class DefaultArguments1 implements BaseInterface1 {
function example($value = NULL);
}

The class implements the interface contract.  This should work.



interface BaseInterface2 {
function example($value);
}

class DefaultArguments2 implements BaseInterface {
function example($value = NULL);
}

The class implements the interface contract.   This should work.



interface BaseInterface3 {
function example();
}

class VariableArguments1 implements BaseInterface3 {
function example() {
if (func_num_args() <= 1) {
...
}
}
}

The class and interface signatures match, but the class has an
implementation twist in that it accepts a variable number of arguments.
This should work.  The interface doesn't specify whether () means no
arguments or variable number of arguments, so one has to assume
variable number.


interface BaseInterface1 {
function example($value = NULL);
}

class VariableArguments2 implements BaseInterface1 {

function example() {
if (func_num_args() <= 1) {
...
}
}
}

This class may implement the contract, too.  Its just hard to tell from
the declarations.  Traditionally in PHP, declarations have not meant
much.  Since interfaces are new, whether or not this works is optional.
if not, then perhaps a varargs keyword will be added in a later
version to help match the declarations.


Now constructors and interfaces...

interface BaseInterface4 {
function __construct();
}

What does it mean to "construct" an interface?  Nothing.  This should
be a compile time error.

Now old style constructors

interface BaseInterface4 {
function OldConstructor1();
}

class OldConstructor1 implements BaseInterface4 {
}

This is an error, too.  The name of a method in the interface cannot
match the name of a class that implements it (or match the name of any
of the parent classes.)  This protects the classes constructors from
becoming involved in an interface.

There is nothing wrong with the interface by iteself.  There is nothing
wrong with the class by itself.  They just cannot be used together.

Moving on the the class side...

Constructors aren't are really an object method, they are a class
method. As such, their signatures should not be a part of any normal
method signature checks.  Here is an example:

class Shape {
function __construct() {}
}

class Polygon {
function  __construct($sides) { parent::_construct(); }
}

class Rectangle {
function __construct($sides, $length, $width) {
parent::_construct($sides); }
}

class Square {
function __construct($sides, $size) { parent::_construct($sides,
$size, $size) }
}

Here each class maintains the is_a relationship with its predecessor.
However, the constructors are essentially unrelated except where
explicitly linked in the code.  The signatures are not linked by the
declaration.  This is correct. PHP should NOT compare the signatures of
the construction methods.  This is consistent with other languages and
consistent with the idea that constructors are class methods, not
instance methods.  This capability is essential.

Actually, unless I am mistaken, constructors ARE just instance methods
in PHP with a fancy naming convention.

Now moving on to the signatures of non-construction methods.

PHP implements a dynamic message passing object paradigm (ala small
talk) instead of a static type based object paradigm (ala java).  This
is a good thing.  The message (method name) is passed up the
inheritance tree until an object says "I can do that."  Only then to
the arguments get bound and the method implementation on the object
invoked.  The checking against number of arguments occurs at binding
time.  This is not bad.  nor is it incorrect.

PHP will never know what types are being passed, so it simulates
overloading with default arguments and variable argument functions.

On Feb 26, 2004, at 1:51 AM, Andi Gutmans wrote:

> Regular inheritance has to follow the same isA rules as interfaces.
> Inheritance is exactly the same thing except that it also has code and
> private/protected methods/members whereas interfaces don't.

regular inheritance does not have to follow the same rules.  interfaces
are an optional type checking device.  Classes can get along just fine
with out the type checking.

PHP is fundamentally a weakly typed dynamically typed language.  A
language that lets you do things like $a = "23" + "5".  has no business
enforcing method signature relationships in class inheritance.  It
hasn't before and shouldn't now.

The message passing paradigm is not wrong.  Type conformance is not the
one true way. Type checking should be strictly optional in PHP, with no
type checking being the default.

> You understand completely. It is incorrect to override methods w/
> incompatible signature. We don't check this in compatibility mode.

If anything, it is the type checking that should go into the
configuration option ghetto.

On Feb 25, 2004, at 5:52 PM, Timm Friebe wrote:

> zend_do_perform_implementation_check() is called from
> do_inherit_method_check() (both in zend_compile.c) which in turn is
> called for inheritance *and* for interfaces. The behaviour is fully
> desirable when implementing interfaces but not for regular inheritance.

I agree.

Michael Walter

22 years ago

Timm Friebe wrote:

> thekid@friebes:~/devel/php/tests > cat inheritance.php
> <?php
>   class Foo {
>     function __construct($foo) {
>     }
>   }
>
>   class Bar extends Foo {
>     function __construct($foo, $bar) {
>       // Add = NULL after $bar to make it work
>     }
>   }
> ?>
> thekid@friebes:~/devel/php/tests > php-dev inheritance.php
>
> Fatal error: Declaration of Bar::__construct() must be compatible with
> that of Foo::__construct() in
> /usr/home/thekid/devel/php/tests/inheritance.php on line 10
>
> Is this really necessary?

If that error still occurs if you explicitely call the parent's
__construct (from the derived __construct), that's a bug IMO.

It's totally understandable to get such an error message in the case the
parent constructor gets called implicitely, but for an explicit call
there should neither be a warning or an error .

Cheers,
Michael