Fix #74872 - Disambiguate the source of a trait alias when possible

[pmmaga]

Link for bugsnet: https://bugs.php.net/bug.php?id=74872

Cases where two traits with a duplicate method name are added in the same use block will still be ambiguous.

[krakjoe]

Please don't include NEWS patches in pull requests, conflicts emerge too quickly ... the merger does the NEWS change when they pull in the PR.

[krakjoe]

@nikic @bwoebi @dstogov can I get a thumbs up from one of you please ?

[pmmaga]

A little more background on each commit and why they are still separate:

• The first one fixes the original issue as it was reported. Meaning that it enables us to know from which trait a method is coming and allowing it to be added to the class if and only if, the receiving class ALSO implements a method with the same name.
• The second one allows the fix to be extended to cases where the receiving class does not implement a method with the same name as the methods brought by the traits. Meaning that aliased methods will no longer collide as it is possible to call them unambiguously.
• The third one enables the better collision message that was commented out on the code. With these fixes it should now be possible to show the better error message on all cases.

I'm not sure if the last one should be applied to GA branches as it will change an error message.

[pmmaga]

I have removed the second commit because allowing a collision when the methods are aliased would still cause issues when that method is being called by it's original name in other methods of the same trait.
As an example:

trait T1 {
    function originalName() {
        echo 'Yeah!' . PHP_EOL;
    }
    
    function hi(){
        $this->originalName();
    }
}

trait T2 {
    function originalName() {
        echo 'No!' . PHP_EOL;
    }
}

class C {
    use T2;
    use T1 {
        originalName as aliasedName;
    }
}

$c = new C();
$c->aliasedName(); // Would output "Yeah!"
$c->hi(); // Would output "No!"

[pmmaga]

Pinging the same that were pinged above. @bwoebi @nikic @dstogov Can any of you review this?

[KalleZ]

@pmmaga are you still actively working on this? It seems like your ping did not do anything, so just putting a friendly nudge here. @nikic perhaps?

[pmmaga]

This needs a rebase as I haven't touched it for a long time. I'll try to get it up to date soon.

[pmmaga]

Freshly rebased on 7.2. I'd like a thumbs up to merge this

[nikic]

At least as implemented this breaks ABI, so can only go into 7.4.

[nikic]

Can use zend_string_equals_ci here.

[nikic]

What happens if we make this use Trait1, Trait3 and Trait3 also has an init method? If I'm reading your code right, it's just going to pick one of them. But I think this should be an error.

I'm wondering if an absolute trait method reference shouldn't be required when multiple traits are used in one block.

[pmmaga]

Right now it still picks one of them in that case. The currrent behavior is to disambiguate when possible and keep the old behavior (pick one) when it's not possible to know. But yes, I think it's a good idea to error in those cases.

I've started porting this to 7.4 but still have to redo the opcache part. So I'll also rework that.

[krakjoe]

@pmmaga can we get a status update here please ?

[nikic]

I've put up an alternative fix at #5232.

[pmmaga]

Closed in favor of the PR above