php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #64235 Insteadof not work for class method in 5.4.11
Submitted: 2013-02-18 11:58 UTC Modified: 2013-02-21 09:12 UTC
Votes:2
Avg. Score:2.5 ± 0.5
Reproduced:1 of 2 (50.0%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: imenem at inox dot ru Assigned: dmitry
Status: Closed Package: Scripting Engine problem
PHP Version: 5.4.11 OS: Debian GNU/Linux
Private report: No CVE-ID:
 [2013-02-18 11:58 UTC] imenem at inox dot ru
Description:
------------
In PHP 5.4.4 test script works as expected, in 5.4.11 script caused fatal error.

Test script:
---------------
<?php

class TestParentClass
{
    public function method()
    {
        print_r('Parent method');
        print "\n";
    }
}

trait TestTrait
{
    public function method()
    {
        print_r('Trait method');
        print "\n";
    }
}

class TestChildClass extends TestParentClass
{
    use TestTrait
    {
        TestTrait::method as methodAlias;
        TestParentClass::method insteadof TestTrait;
    }
}

(new TestChildClass)->method();
(new TestChildClass)->methodAlias();

Expected result:
----------------
Parent method
Trait method

Actual result:
--------------
Fatal error: Trait TestParentClass is not used in test.php on line 28

Patches

deny_use_with_classes.patch (last revision 2013-02-21 09:09 UTC) by laruence@php.net)
bug64235.patch (last revision 2013-02-20 08:07 UTC) by laruence@php.net)
bug64235.phpt (last revision 2013-02-20 07:41 UTC) by laruence@php.net)

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-02-20 07:00 UTC] reeze@php.net
Hi dmitry, 
  will you take a look please.
 [2013-02-20 07:00 UTC] reeze@php.net
-Assigned To: +Assigned To: dmitry
 [2013-02-20 07:15 UTC] laruence@php.net
actually, I was looking into this,  I have thought the similar fix of you, but 
unfortunatly, it is wrong, thinking of this:

<?php
class TestParentClass
{
    public function method()
    {
        print_r('Parent method');
        print "\n";
    }
}

trait TestTrait
{
    public function method()
    {
        print_r('Trait method');
        print "\n";
    }
}

class TestChildClass extends TestParentClass
{
    use TestTrait
    {
        TestParentClass::method as methodParent;
    }
}

(new TestChildClass)->methodParent();
~
 [2013-02-20 07:39 UTC] laruence@php.net
The following patch has been added/updated:

Patch Name: bug64235.patch
Revision:   1361345980
URL:        https://bugs.php.net/patch-display.php?bug=64235&patch=bug64235.patch&revision=1361345980
 [2013-02-20 07:41 UTC] laruence@php.net
The following patch has been added/updated:

Patch Name: bug64235.phpt
Revision:   1361346083
URL:        https://bugs.php.net/patch-display.php?bug=64235&patch=bug64235.phpt&revision=1361346083
 [2013-02-20 07:44 UTC] laruence@php.net
fix attached, which fix this bug, and trigger fatal error in compile time for the 
test script I pasted before.
 [2013-02-20 07:58 UTC] dmitry@php.net
-Status: Assigned +Status: Feedback
 [2013-02-20 07:58 UTC] dmitry@php.net
Yet another traits mess :(
I suppose, it worked in 5.4.4 because of mistake.
RFC and traits documentation don't say if class names may be used in context of "as" and "insteadof" operators.

In my opinion, class names must not be used in contest of "as".
However, their usage in context of "inseadof" may make sense (I'm not sure).
Anyway, it has to be defined in documentation first.
 [2013-02-20 08:00 UTC] reeze@php.net
insteadof and 'as' bother for traits, I thought after the trait refactor, it's 
works as expected.

If we keep 'insteadof' could been used for class method as feature I'm fine:0
 [2013-02-20 08:04 UTC] dmitry@php.net
It's hard to say what is expected :)
I thought only traits may be used in context of "insteadof", now I'm not sure.

I sent the question to Stefan Marr. Lets wait for his opinion.
 [2013-02-20 08:07 UTC] laruence@php.net
The following patch has been added/updated:

Patch Name: bug64235.patch
Revision:   1361347678
URL:        https://bugs.php.net/patch-display.php?bug=64235&patch=bug64235.patch&revision=1361347678
 [2013-02-20 08:12 UTC] laruence@php.net
form the context,  insteadof works at class make sense.

reeze, whatever the RFC is, your fix simply skip check for classes at all, which 
will make the test script I paste result in "FATAL ERROR, undefined method", that 
is not acceptable.
 [2013-02-20 08:16 UTC] reeze@php.net
@laurence  the code you paste above works the same as before:
http://3v4l.org/UpMCW#v5411 that didn't break

After read some doc I assume class is not suitable in context of 'insteadof', 
so let's wait for Stefan or more discussion :)
 [2013-02-20 08:22 UTC] laruence@php.net
reeze, *before* doesn't always means *RIGHT*.
 [2013-02-20 13:20 UTC] gron@php.net
Hi:

The `insteadof` and `as` operators where not intended to be used with classes.
The syntax is intended to convey that the use operation is refined by specifying how to 
resolve conflicts __between__ traits.
That's the idea at least.

My solution for the initial problem presented would be to provide a method such as follows in 
the TestChildClass:
  public function method() {
    parent::method();
  }

I understand that this is not ideal, and requires you to repeat yourself.
However, it is consistent in the sense that traits are traits and not classes, and both get 
mixed up as little as possible,

However, beside the academic notion of purity, I can see that `TestParentClass::method 
insteadof TestTrait;` is useful.
[I wonder whether `parent::method insteadof TestTrait;` should be supported as well.]


Laruence's example with `TestParentClass::method as methodParent;` is however problematic. 
Traits are not supposed to conflict with classes, but with traits. So, allowing the 
introduction of aliases for method of the super class seems to me as something that is 
problematic, because it mixes up the concepts.

If you need an alias for the method of a parent class, the classic way would be:

public function foo() {
  parent::bar();
}

No?


Well, that's my point of view.


So, from a practical point of view, referring to the parent (and only the direct parent) class 
in `insteadof` might be a useful/acceptable feature.
The use in conjunction with `as` seems to be something I would advocate against.
In either way, beside bugs that made this possible in the first place, I would consider both 
ideas as new features that need to be documented/discussed.

I thought that we had a test that only the traits listed in the `use` clause can be used for 
the `as`/`insteadof` operators, but that's either broken or not there, or a bug that was fixed 
in 5.4.11 as the original report suggests.
Therefore, from my perspective, 5.4.11 shows the behavior that's intended by the spec/RFC.

Best regards
Stefan
 [2013-02-20 14:40 UTC] laruence@php.net
@Stefan the current patch is allow use insteadof with classes (as the reporter 
said, it used to works), 

and forbid use "as" with classes (5.4.11 will result in an unexpected FATAL 
error "undefined method", which is very confused message), the new solution is 
trigger compiler ERROR

so, for the current patch, I think it is consistent with before, no need to be 
RFCed again.

however, if we decide to forbind using both 'as' and 'insteadof' with classes, 
that definitely need a RFC
 [2013-02-20 16:22 UTC] reeze@php.net
currently using class in context of trait 'as' and 'inteadof'
didn't work most of the time (as the code below demonstrated, even after apply the patch
larucence attached). Only the use case of this bug report, it HAPPENED to work. 


To keep the BC of the problem reported, as I suggested in the previous patch we could 
mark it as deprecated in 5.4.

So forbid class in 'as' and 'insteadof' didn't break because most of them didn't even work.

in the case of parent class insteadof usage, the REAL need is avoid trait's method overwrite
the method inherited but not refer to parent class.

there is a really easy workaround: rename the conflict method as
a new one `Traits::method as _use_less` or something else, 
if we really need the method from parent.


<?php

class Standalone {
	function foo() {
		echo "I shouldn't been called";
	}
}

class GrandParent {
	function foo() {
		echo "GrandParent";
	}
}

class Parent extends GrandParent {
	function foo() {
		echo "Parent";
	}	
}

trait T {
	function foo() {
		echo "Trait";
	}
}

class Child extends Parent {
	use T {
		/* as */
		Parent::foo as bar;		// Child::bar() -> undefined method
		StandAlone::foo as bar; // Child::bar() -> undefined method
		StandAlone::foo as foo; // Child::foo() -> "Trait"
		GrandParent::foo as bar; // Child::bar() -> undefined method

		/* insteadof */
		Parent::foo insteadof T; // works by accident -> "Parent"
		GrandParent::foo insteadof T; // -> "Parent" but not "Grand"
		StandAlone::foo insteadof T; // ->"Parent"
	}
}
 [2013-02-20 19:33 UTC] dmitry@php.net
I agree with Stefan.
I think we have to allow only trait names in context of "as" and "insteadof" statements. Stefan showed a simple workarounds for class names.
I don't think we should fix the behavior we had in early 5.4 versions by mistake.
 [2013-02-21 09:09 UTC] laruence@php.net
The following patch has been added/updated:

Patch Name: deny_use_with_classes.patch
Revision:   1361437796
URL:        https://bugs.php.net/patch-display.php?bug=64235&patch=deny_use_with_classes.patch&revision=1361437796
 [2013-02-21 09:12 UTC] dmitry@php.net
Personally, I agree with deny_use_with_classes.patch
 [2013-02-21 10:20 UTC] laruence@php.net
-Status: Feedback +Status: Closed
 [2013-02-21 10:20 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=9a44a9806c7c7a8c1fc691210335d0691a4597be
Log: Fixed bug #64235 (Insteadof not work for class method in 5.4.11)
 [2013-02-21 10:21 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=9a44a9806c7c7a8c1fc691210335d0691a4597be
Log: Fixed bug #64235 (Insteadof not work for class method in 5.4.11)
 [2013-02-21 10:21 UTC] laruence@php.net
Automatic comment on behalf of laruence
Revision: http://git.php.net/?p=php-src.git;a=commit;h=9a44a9806c7c7a8c1fc691210335d0691a4597be
Log: Fixed bug #64235 (Insteadof not work for class method in 5.4.11)
 [2013-04-02 15:02 UTC] maciej dot sz at gmail dot com
Is there going to be a follow up discussion regarding the necessity of repeating methods in case traits and classes collide?

Stefan mentioned: "I understand that this is not ideal, and requires you to repeat yourself", but I don't think that this statement grasps the magnitude of the problem. You bring up only simplest cases here - methods with no parameters. But in reality we, PHP users, write methods with fair amount of parameters and a lot of docblock text. Should we repeat it all? Traits are meant to decrease repetition, right?
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Thu Apr 17 16:02:22 2014 UTC