Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Resolve deprecated tag also from parents" #1022

Closed
wants to merge 1 commit into from

Conversation

ossinkine
Copy link
Contributor

This reverts #792, see #792 (comment)

@ondrejmirtes
Copy link
Member

Sorry, I like the new behaviour more, it makes more sense to me.

@ossinkine
Copy link
Contributor Author

Сlass can have its own methods (not inherited from an abstract class or interface). If a method is deprecated in an interface, but the class still wants to have such a method (without implementing the interface), what should be done in this case? According to your logic, such a method should also be deprecated and the class cannot have it on its own. If a class has a method that only implements an interface, then the method in the class must also be marked as deprecated, because it will be removed in the next major release.

I have already given an example with Doctrine. In the next major release Doctrine\Persistence\Event\LifecycleEventArgs::getEntity will be deleted but Doctrine\ORM\Event\LifecycleEventArgs::getEntity is not, so why it should be deprecated?

Symfony following the same way - if the method is deprecated in the implemented interface and won't be used itself it either marked as deprecated (f.e. Symfony\Component\Security\Http\Authenticator\HttpBasicAuthenticator::createAuthenticatedToken) or marked as inheritdoc that also implies that it is deprecated (f.e. Symfony\Component\Security\Core\User\ChainUserProvider::loadUserByUsername).

@ondrejmirtes could you explain your point?

@ondrejmirtes
Copy link
Member

Please discuss with @eiriksm who contributed the counter-PR in #792 :)

@eiriksm
Copy link
Contributor

eiriksm commented Mar 2, 2022

Hi there.

I must say it sounds strange to me to implement a method that is marked as deprecated, and then expect that method itself to not be deprecated. I see your examples, and I understand that it happens and is technically possible. But it sounds strange to me, and I would personally refrain from using a method that is deprecated on the interface level (for example the ones in your examples)

I can agree that a middle ground would be to only expect it when the class specifies inheritdoc from the (deprecated) interface method. But I disagree with removing it altogether. I would expect these cases to be few enough that you can simply ignore them in your phpstan config?

I can also contribute my own story about this (and why I contributed the code in the first place). I was working on a Drupal project where the TermInterface::getVocabularyId method was deprecated in Drupal 8. The code in question was type hinting an implementation (Term class). When this project was upgraded to Drupal 9 this method very surprisingly was not existing. Why on earth did I not get a warning, was it not deprecated, I thought? Well it was. Then why didn't phpstan warn me? Well... Hence the PR. In drupal it's most common to deprecate methods on the interface and then remove them. So yeah, I need this 🤓

@ondrejmirtes
Copy link
Member

I agree with @eiriksm here. I suggest @ossinkine to rename the getEntity method to something else to note that it has nothing in common with the deprecated one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants