Skip to content

Conversation

@TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Nov 16, 2024

The Override interface makes sense only overriden parent class method (or trait one), so we know it's change is on purpose.
It doesn't make sense to add override for interface, as all interface methods would be marked :)

@TomasVotruba TomasVotruba marked this pull request as draft November 16, 2024 21:09
@TomasVotruba TomasVotruba changed the title Fix interface implementer in AddOverrideAttributeToOverriddenMethodsRector Fix interface implementer in AddOverrideAttributeToOverriddenMethodsRector, add override only in case of overiding parent method with contents Nov 16, 2024
@TomasVotruba TomasVotruba force-pushed the tv-overriden-skip-interface branch 4 times, most recently from d8e5a5d to f36e340 Compare November 16, 2024 23:35
@TomasVotruba TomasVotruba force-pushed the tv-overriden-skip-interface branch from f36e340 to 9469eeb Compare November 16, 2024 23:36
@TomasVotruba TomasVotruba marked this pull request as ready for review November 16, 2024 23:36
@TomasVotruba TomasVotruba merged commit 26b4d97 into main Nov 16, 2024
@TomasVotruba TomasVotruba deleted the tv-overriden-skip-interface branch November 16, 2024 23:38
public function foo()
{
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the foo and bar override from trait usage in on purpose check, as foo() is override empty, while bar() is not, which cause error, see https://3v4l.org/J50fsl

Copy link
Member

@samsonasik samsonasik Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the proof https://3v4l.org/O2TdJ, I will add check back for it, now got error:

Fatal error: ApplyAttributeToOverrideMethodFromTrait::bar() has #[\Override] attribute, but no matching parent method exists in /in/O2TdJ on line 16

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants