Page MenuHomePhabricator

Helper for deprecating method overrides
Closed, ResolvedPublic

Description

  1. Create a helper trait to easily add warnings for methods that should not be overridden anymore.
  2. T267085: Clarify deprecation of method overrides in the stable interface policy

Event Timeline

Change 638202 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] debug: Add DeprecateOverridesHelper trait

https://gerrit.wikimedia.org/r/638202

Demian triaged this task as Medium priority.Nov 3 2020, 4:42 AM
Demian updated the task description. (Show Details)

Assuming this task is about the MediaWiki core codebase.

[Resetting assignee due to inactive user account]

We not only need to trigger a deprecation warning if the method is overridden, we also need to call the method if it is overridden, and not if not.

I think we could establish a pattern as follows:

class TheBaseClass {

  public function sillyOldMethod() {
    wfDeprecated( __METHOD__, '1.36' );
    //... do silly things...
  }

  public function someMethod() {
    // This is similar to using Hooks::isRegistered() for calling deprecated hooks.
    if ( MWDebug::checkForDeprecatedMethodOverride( $this, __CLASS__, __FUNCTION__, '1.36' ) ) {
        $this->sillyOldMethod();
    }
  }

}
class SomeExtensionClass extends TheBaseClass {
  public function sillyOldMethod() {
    //... do silly extension things, not knowing about deprecation...

    // calling the parent method will trigger another deprecation warning.
    parent::sillyOldMethod();
  }

  public function funMethod() {
    // will only trigger a deprecation warning if the parent method is called above.
    $this->sillyOldMethod();
  }

}
class AnotherFunExtensionClass extends TheBaseClass {

  public function funMethod() {
    // will trigger a deprecation warning as normal
    $this->sillyOldMethod();
  }
}

Note that we could call checkForDeprecatedMethodOverride in the constructor as well, so we catch overrides in classes that also override someMethod(). But we don't need to. They wouldn't break when we remove the method from the base class.

class YetAnotherExtensionClass extends TheBaseClass {

  public function sillyOldMethod() {
    // ...do fun extension things, not calling parent...
  }

  public function someMethod() {
    // This will not trigger any deprecation warning.  And it doesn't have to, it can just stay as it is.
    $this->sillyOldMethod();
    // ...do more fun things...
  }

}

Moving this from the code review board to the PET inbox, because this task has merit, but the author of the task and patch has been blocked for unrelated reasons.

Whether a method has been overridden can easily be detected using reflection. This is about 30 minutes of work, and we should just do it.

I don't think hard deprecation should be necessary prior to removal, if the thing being removed is not a callable function. You can deprecate pretty much any pattern or concept, but such things are not necessarily amenable to hard deprecation warnings. I think a deprecation is fundamentally an announcement, warning of the future removal of some functionality. We can't guarantee that all such announcements will appear in your debug logs.

I don't think hard deprecation should be necessary prior to removal, if the thing being removed is not a callable function. You can deprecate pretty much any pattern or concept, but such things are not necessarily amenable to hard deprecation warnings. I think a deprecation is fundamentally an announcement, warning of the future removal of some functionality. We can't guarantee that all such announcements will appear in your debug logs.

You are right that we can't guarantee generating warnings for the removal for any kind of concept or behavior, but I think we should generate such warnings if reasonably possible. This seems to be the case here.

The stable interface policy says:

Stable to override can apply to class methods and hooks. It means the subclass method or hook handler will continue to be called in relevant circumstances to let you influence the caller's behaviour. Changes to that contract must follow the deprecation policy.

it further says that:

Code MUST emit hard deprecation notices for at least one major MediaWiki version before being removed

It's not directly requiring deprecation warnings to be emitted when removing an overridable method, but I does seem to imply that this is the case. I'd certainly interpret the policy that way.

Just the other day I filed T268326: RFC: Amendment to the Stable interface policy (November 2020). Perhaps comment there if you think this should change or be clarified somehow.

Change 642847 had a related patch set uploaded (by Jdlrobson; owner: Tim Starling):
[mediawiki/core@master] Revert "Deprecate Skin::setupSkinUserCss"

https://gerrit.wikimedia.org/r/642847

Change 644931 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Emit deprecation warning for deprecated overrides.

https://gerrit.wikimedia.org/r/644931

Change 644931 merged by jenkins-bot:
[mediawiki/core@master] Emit deprecation warning for deprecated overrides.

https://gerrit.wikimedia.org/r/644931

daniel claimed this task.