php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #76370 Fatal error instead of warning when extending class that implements interface
Submitted: 2018-05-24 08:37 UTC Modified: 2018-08-05 09:56 UTC
From: kinglozzer at gmail dot com Assigned:
Status: Not a bug Package: Class/Object related
PHP Version: 7.2.5 OS: Mac OS X
Private report: No CVE-ID: None
 [2018-05-24 08:37 UTC] kinglozzer at gmail dot com
Description:
------------
If a class implements an interface, any child classes method definitions must match the *parent class* exactly (not the interface) or a fatal error will occur.

Unexpected behaviour - fatal error: https://3v4l.org/BeFp5

Expected behaviour - warning: https://3v4l.org/ZhHrE (without implementing an interface).

The introduction of an interface to the base class should not affect the behaviour of child classes.

Test script:
---------------
https://3v4l.org/BeFp5

Expected result:
----------------
Warning: Declaration of Bar::method() should be compatible with Foo::method($arg = NULL)

Actual result:
--------------
Fatal error: Declaration of Bar::method() must be compatible with Foo::method($arg = NULL)

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-08-05 07:58 UTC] Wes dot example at example dot org
Not only it shouldn't be a fatal error, but shouldn't be a warning either. Parameters are optional and untyped in all the signatures, hence going to one to the other should never cause errors. That is because php does not reject trailing parameters.

function bar(mixed $one = null, mixed ...$foo){}
function bar(mixed $one = null, mixed $two = null){}
function bar(mixed ...$foo){}
function bar(){}

these are all compatible... because no parameter is ever required, in all of them
 [2018-08-05 08:18 UTC] Wes dot example at example dot org
(assuming mixed includes all types including null)

class Foo {
    public function method(?X $arg = null) {}
}

class Bar extends Foo {
    public function method(){}
    // PHP should interpret this the same as
    // public function method(mixed $arg = null){}
}

class Baz extends Bar {
    // ...in order to disallow stuff like this:
    public function method(?Y $arg = null) {}
    // Error: parameter 1 must be mixed and be optional as in class Bar
}
 [2018-08-05 09:13 UTC] requinix@php.net
-Status: Open +Status: Not a bug
 [2018-08-05 09:13 UTC] requinix@php.net
Not a bug.

It is not compatible because the parent implementation supports an argument while the child does not.

https://en.wikipedia.org/wiki/Liskov_substitution_principle
LSP says that method arguments must be contravariant, meaning that a child method must accept at least as much as its parent did. If the parent supports having an argument then the child must also support having an argument. That argument can be made optional in the child, but it must still be there.


> The introduction of an interface to the base class should not affect the behaviour of child classes.
I agree, but life isn't that simple: where the method was defined does matter. Dropping parameters used to be allowed in PHP 4's lackluster implementation of OOP, so during PHP 5's overhaul it became an E_STRICT-able offense and later an E_WARNING. It remains a warning for backwards compatibility. It shouldn't be allowed at all, though, so when defined in a interface (introduced in PHP 5) it was immediately considered a fatal error.
 [2018-08-05 09:56 UTC] requinix@php.net
> these are all compatible...
There's more to compatibility than simply whether you can get locate the values somewhere in the arguments. There's meaning to how parameter lists are constructed:
1. bar($one=null, ...$foo) has one normal arg then whatever else after is extra and arbitrary
2. bar($one=null, $two=null) has two normal args
3. bar(...$foo) has arbitrary arguments
4. bar() has no arguments at all

All four of those are different; a normal argument has some deliberate meaning behind its inclusion in the signature, while variadics [are syntactic sugar for an array, but otherwise] indicate some set of values whose meanings are indistinct from each other.

I'm big on coding theory so that's how I think about it. If you want something more concrete, consider a method
  public function bar($one, ...$foo) {
that is correctly extended in a child as
  public function bar($one, ...$foo)
Now the parent adds a second parameter before the variadic list.
  public function bar($one, $two, ...$foo)
The child should not be compatible anymore because a call with an argument intended for $two would instead end up in the $foo array when it was clearly meant to have whatever particular significance necessitated $two's creation as a separate argument. You might very well end up wanting to array_unshift $two into $foo, of course, but there's no way PHP could know that.


> // PHP should interpret this the same as
PHP should interpret what was written. It can't tell the difference between a developer forgetting that the method accepts an $arg and a developer deliberately ignoring the argument. Or a developer omitting the argument in favor of func_get_arg/s. Or a developer intentionally creating a syntax error as a reminder that the code isn't finished yet.

If you knew what you were doing and did, in fact, want to ignore the parameter, then that's what you should do: define the parameter so that PHP recognizes you're aware of what the signature is intended to be, and ignore it.
 [2018-08-05 11:37 UTC] spam2 at rhsoft dot net
these should be at least only warnings without any but or if

fatal errors making maintainance of code difficult with no need - frankly such sort of fatal errors made me crazy by introduce type hints and strict types all over the company
 [2018-08-11 09:21 UTC] kinglozzer at gmail dot com
Thanks for the explanation requinix, I agree that this is a symptom of code that *really* should be refactored.

However this behaviour (triggering a fatal error) was only introduced in PHP 7.2.0, and isn’t documented anywhere, which leads me to believe that it was an unintentional side-effect of another change (possibly the parameter type widening feature?). A little context on the source of this: https://github.com/silverstripe/silverstripe-cms/pull/2174.

I’m not entirely familiar with how PHP deals with backward compatibility issues, so I’ll leave it to your judgement to decide whether or not this is a valid bug! Thanks :)
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Mon Jul 06 02:01:28 2020 UTC