php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #55554 Trait methods overriding legacy constructors
Submitted: 2011-08-31 15:49 UTC Modified: 2011-10-16 18:45 UTC
From: ryan at zuttonet dot com Assigned:
Status: Closed Package: Scripting Engine problem
PHP Version: 5.4.0alpha3 OS: Ubuntu
Private report: No CVE-ID: None
 [2011-08-31 15:49 UTC] ryan at zuttonet dot com
Description:
------------
For the sake of consistency, exactly one of the following should be implemented:

1. Trait methods should be able to override __construct definitions
2. Trait methods should not be able to override legacy constructor definitions

Currently, trait methods are not able to override __construct 
definitions. Trait methods are able to override legacy constructor definitions.

Test script:
---------------
Here are two test cases that will reproduce the defect:

https://gist.github.com/1183844

Expected result:
----------------
A trait-level __construct method (or a trait-level method aliased as __construct) 
should not be able to override any type of constructor in a class

Actual result:
--------------
Fatal error: Call to private SomeClass::__construct() from invalid context

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-08-31 15:52 UTC] ryan at zuttonet dot com
Apologies; the literal expected output for the provided test scripts should be:

"this is a constructor"

As this is the output when using __construct() in the class definition instead of 
a legacy-style constructor.
 [2011-09-12 10:47 UTC] gron@php.net
Hi Ryan:

I am sorry, I don't think I understand what inconsistency you are pointing out.
Could you elaborate on the problem?

(And please include the code you are referring to directly here. Just to make it 
easier for me 
to know that we are talking about exactly the same code.)

What I understand is you want to provide the constructor with a trait, right?

Like this:

<?php

trait MyTrait {
    public function constructor() {
        echo "Foo\n";
    }
}

class MyClass {
	use MyTrait {
	    constructor as __construct;
	}
}

class MyClass2 {
	use MyTrait {
	    constructor as MyClass2;
	}
}

echo "MyClass constructor: ";
$o = new MyClass;   // echos Foo

echo "\nMyClass2 constructor: ";
$o = new MyClass2;  // doesn't echo Foo
echo "\n";
?>

The problem I see here is that for MyClass2 the constructor does not actually 
get registered 
as a constructor but just as a normal method.
That seems to be an inconsistency that needs to be fixed.

Ok, now to the new vs. legacy constructors:

class Bar {
    function Bar() {
        echo "BarBar new ctor\n";
    }
    
    function __construct() {
        echo "Bar new ctor\n";
    }
}

$o = new Bar;
?>

Switching the order of the definition of the constructors doesn't influence the 
result, 
__construct always wins.


Both your examples behaves identical to the situation if the __construct would 
have been 
defined directly in the class. So, where is the problem here?
It is not an inconsistency with how PHP behaves without traits, from what I can 
see.

Ah, and please try the latest code in the SVN, I am not exactly sure whether I 
changed 
anything that could be related to that between alpha3 and now. But I doubt it.

Thanks
Stefan
 [2011-09-12 10:48 UTC] gron@php.net
-Status: Open +Status: Assigned -Package: *Programming Data Structures +Package: Scripting Engine problem -Assigned To: +Assigned To: gron
 [2011-09-12 17:38 UTC] ryan at zuttonet dot com
Thanks, Stefan (the defect directions explicitly say not to include examples 
over 20 lines; that's why I provided the github link)

The real issue is this: should we be allowed to define, in a trait, a method 
that will override the constructor of the class into which we're importing the 
trait?

I think not, but currently it can be done, although not consistently.

Example 1:

<?php
/**
 * ============
 * Defect: can alias-override class constructor, but only when using legacy 
constructor naming convention
 * ============
 */
trait SomeTrait {
	private function constructor() {
		echo "this is abuse\n";
	}
}

class SomeClass {
	use SomeTrait {
		constructor as __construct;
	}

	public function SomeClass() {
		echo "this is a constructor";
	}
}

$c = new SomeClass(); // Fatal error: Call to private SomeClass::__construct()

In this example, I can override SomeClass::SomeClass() as the constructor by 
importing SomeTrait::constructor() as SomeClass::__construct(). However, this 
works only because I'm using the legacy constructor SomeClass::SomeClass(). If I 
had defined SomeClass::__construct() in SomeClass' definition, PHP would not 
allow this trait abuse.

Example 2:

<?php
/**
 * ============
 * Defect: can override class constructor, but only when using legacy 
constructor naming convention
 * ============
 */
trait SomeTrait {
	private function __construct() {
		echo "this is abuse\n";
	}
}

class SomeClass {
	use SomeTrait;

	public function SomeClass() {
		echo "this is a constructor";
	}
}

$c = new SomeClass(); // Fatal error: Call to private SomeClass::__construct()

In this case, I can override the legacy constructor SomeClass::SomeClass() with 
a method name SomeTrait::__construct(). However, if I had a method named 
SomeClass::__construct(), it would not be overridden by the trait method.

Do my concerns make more sense now? It's not about what _should_ be allowed, 
it's about the inconsistency in what currently is allowed.

In order to be consistent, either both types of constructors should be 
overridable, or neither should (probably the latter).

Again, it's about the overriding of an existing constructor, not about the 
mixing in of one into a class with no constructor.
 [2011-09-12 19:27 UTC] gron@php.net
Hi Ryan:

(I wonder whether I could answer directly to those emails sent be the tracker, 
this here manually is rather messy ...)


My main point is, that PHP does handle classes that define both kinds of 
constructors in a very specific way. And that is consistent with how it is handled 
when traits are used.

The answer to your question, whether a trait should be able to override the 
constructor of a class in which it is used, is no. The class definition should 
always take precedence. However, the problem here is deeper. Since legacy 
constructor and __construct are not considered equal, we got a problem.

The inequality allows you to define both in a class, as my previous example 
showed. And well, for the same reason it also works with traits, of course with 
unintended side effects.

I will see what pops up when I fix the issue that you can not define legacy 
constructors with a trait. I will probably add a test that constructors are 
handled more carefully in traits then. The WTF factor is much higher and not worth 
keeping the consistency, I think.

Best regards
Stefan
 [2011-09-14 06:17 UTC] ryan at zuttonet dot com
Thanks for the clarification; while that answer isn't very satisfying, it's sort 
of hard to argue when the problem is so deeply rooted in the language.

Is this inconsistency in constructor handling a design decision or a fundamental 
error in the engine? Are there any plans in place to enforce consistency? 

If you're going to add those tests, then I guess there's not much left to do here.
 [2011-10-09 11:13 UTC] gron@php.net
Automatic comment from SVN on behalf of gron
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=317935
Log: Fixed Bug #55554 (Legacy constructors not handled properly) [TRAITS] [DOC]
# The handling of legacy constructors defined by traits was corrected.
# They are now properly registered and used on instantiation.
# The situation for conflicting legacy and __construct constructors is
# mostly identical. If they are defined in the class, they override conflicts
# and do not collide. However, in case different styles are mixed, between
# class and trait definition, we assume a programmer's mistake and report
# a collision.
#
# BTW: +1 for all the fixed tests! `make test` is fun again.
 [2011-10-09 11:16 UTC] gron@php.net
-Status: Assigned +Status: Open
 [2011-10-09 11:16 UTC] gron@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 [2011-10-09 11:17 UTC] gron@php.net
-Status: Assigned +Status: To be documented
 [2011-10-09 11:17 UTC] gron@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 [2011-10-09 11:17 UTC] gron@php.net
-Status: To be documented +Status: Assigned
 [2011-10-09 11:17 UTC] gron@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 [2011-10-09 11:17 UTC] gron@php.net
-Status: Assigned +Status: Open -Assigned To: gron +Assigned To:
 [2011-10-09 11:18 UTC] gron@php.net
-Status: Open +Status: To be documented
 [2011-10-09 11:18 UTC] gron@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 [2011-10-16 18:45 UTC] gron@php.net
-Status: Open +Status: To be documented
 [2011-10-16 18:45 UTC] gron@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 [2012-04-18 09:48 UTC] laruence@php.net
Automatic comment on behalf of gron
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7e4b9800f48dc43e4d65afb70fcca171a7ac0def
Log: Fixed Bug #55554 (Legacy constructors not handled properly) [TRAITS] [DOC] # The handling of legacy constructors defined by traits was corrected. # They are now properly registered and used on instantiation. # The situation for conflicting legacy and __construct constructors is # mostly identical. If they are defined in the class, they override conflicts # and do not collide. However, in case different styles are mixed, between # class and trait definition, we assume a programmer's mistake and report # a collision. # # BTW: +1 for all the fixed tests! `make test` is fun again.
 [2012-07-24 23:39 UTC] rasmus@php.net
Automatic comment on behalf of gron
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7e4b9800f48dc43e4d65afb70fcca171a7ac0def
Log: Fixed Bug #55554 (Legacy constructors not handled properly) [TRAITS] [DOC] # The handling of legacy constructors defined by traits was corrected. # They are now properly registered and used on instantiation. # The situation for conflicting legacy and __construct constructors is # mostly identical. If they are defined in the class, they override conflicts # and do not collide. However, in case different styles are mixed, between # class and trait definition, we assume a programmer's mistake and report # a collision. # # BTW: +1 for all the fixed tests! `make test` is fun again.
 [2013-11-17 09:36 UTC] laruence@php.net
Automatic comment on behalf of gron
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7e4b9800f48dc43e4d65afb70fcca171a7ac0def
Log: Fixed Bug #55554 (Legacy constructors not handled properly) [TRAITS] [DOC] # The handling of legacy constructors defined by traits was corrected. # They are now properly registered and used on instantiation. # The situation for conflicting legacy and __construct constructors is # mostly identical. If they are defined in the class, they override conflicts # and do not collide. However, in case different styles are mixed, between # class and trait definition, we assume a programmer's mistake and report # a collision. # # BTW: +1 for all the fixed tests! `make test` is fun again.
 [2013-11-17 09:36 UTC] laruence@php.net
-Status: Open +Status: Closed
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Wed Jul 17 08:01:26 2019 UTC