Page MenuHomePhabricator

RFC: Establish stable interface policy for PHP code
Closed, ResolvedPublic

Description

This RFC proposes policies and strategies to mitigate problems arising from the need to change PHP interfaces that are implemented by classes defined by extensions. The core of the proposal is to recommend the use of base classes instead, and be more explicit about what constitutes MediaWiki's stable interface, and what guarantees apply to which part of it.

Problem

Historically, the main contact surface between MediaWiki and extensions were base classes (to be subclassed by extensions) and hook signatures. This made non-breaking changes (with support for outdated code during some deprecation period) relatively easy: it was possible to add more parameters to a hook signature, add more parameters (with default values) to a base class method, and add new methods to a base class, without breaking anything, since all of these are valid in PHP:

function hook1( $a ) {}
call_user_func( 'hook1', 1, 2 );
class Core1 {
    public function f( $a, $b = null ) {}
}
class Extension1 extends Core1 {
    public function f( $a ) {}
}
// will raise a warning but still work
class Core2 {
    public function f( $a ) {}
    public function f2( $b ) {}
}
class Extension2 extends Core2 {
    public function f( $a ) {}
}

With dependency injection and generic composition over inheritance practices, we are increasingly moving towards asking extensions to implement an interface instead of subclassing a base class (and there are plans to change the hook system in similar ways). The above change mitigation strategies fail for interfaces:

interface Core3 {
    public function f( $a, $b = null );
}
class Extension3 implements Core3 {
    public function f( $a ) {}
}
// fatal error
interface Core4 {
    public function f( $a );
    public function f2( $b );
}
class Extension4 implements Core4 {
    public function f( $a ) {}
}
// fatal error

Replacing the old interface with a new one is not much better as it will break type hints. If we want to avoid inflicting a lot of pain on wiki owners with in-house or not fully up to date extensions, we need to come up with new change mechanisms.

Proposal (Daniel, September 2019)

The problem outlined above is best addressed by using (abstract) base classes instead of "proper" PHP interfaces for extension points, and by providing more explicit guarantees to extension authors. The following steps are proposed:

  1. Adopt the Stable Interface Policy as drafted at https://www.mediawiki.org/wiki/Stable_Interface_Policy. The policy will take effect with the release of MW 1.35, giving extension authors time to adapt. This new policy will replace the existing Deprecation policy.
  2. Before the release of MW 1.35, add the annotations defined by the Stable Interface Policy to the relevant classes in MediaWiki core. Use existing extensions as a guide for what needs the new annotations.

Summary of the new policy

For extension authors:

  • It's generally safe to call public methods, and to access public fields in classes defined by MediaWiki core, unless these methods are documented to be unsafe (e.g. annotated as @deprecated, @unstable, or @internal).
  • It's generally unsafe to extend (subclass) or implement interfaces defined by MediaWiki core, unless that class or interface was marked as safe for that purpose. In particular, the constructor signature may change without notice, and abstract methods may be added to interfaces.
  • It's generally unsafe to directly instantiate (using new) classes defined by MediaWiki core, unless that class is marked as @newable.
  • It's generally unsafe to rely on global variables from MediaWiki core. Use methods such as MediaWikiServices::getInstance() or MediaWikiServices::getMainConfig() instead.

When changing existing code:

  • Keep public methods and hook signatures backwards compatible for callers. Follow the deprecation process when removing them.
  • Keep constructor signatures backwards compatible if the constructor was marked @stable for calling.
  • Ensure compatibility of method signatures for code that overrides them if they are marked @stable for overriding.
  • Do not add abstract methods to classes or interfaces marked as @stable for subclassing or @stable for implementation.

When defining extension points:

  • When defining hooks, keep the signature minimal, and expose narrow interfaces, ideally only pure value objects.
  • When defining an interface to be implemented by extensions, provide a base class, and mark it as @stable for subclassing.
  • Discourage extensions from directly implementing interfaces by marking them as @unstable for implementation. If direct implementation is to be allowed, mark the interface @stable for implementation.

Notable changes from the 1.34 policy:

  • Public methods are per default considered stable only for calling, not for overriding.
  • Constructors are considered unstable per default.
  • Classes and interfaces are considered unstable for subclassing and implementation, unless documented otherwise.
  • Code not used in a public repository that is part of the Wikimedia ecosystem may be changed or removed without deprecation

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Osnard added a comment.Jul 2 2018, 6:40 AM

Sorry, I did not have the time to read the whole discussion, so maybe this is misses the point. But as an extension developer I'd really love to see more (abstract) base classes that I can rely on. It helps me with the decision of how to structure my code.

For example, in BlueSpice we've created abstract base classes for a range of frequently used hooks [1][2]. When a developer wants to bind to a hook he only needs to extend the appropriate base class [3] and register the callback [4]. He does not need to care about the signature of the hook, as the hook base class provides him with the hooks' arguments in form of protected properties [5]. Some base classes even implement convenience methods [6]. If a hook signature changes, the base class can adapt to it and maybe provide a "compatibility layer/shim" to the subclasses (e.g. if the hook signature is not just extended, but a parameter actually changes; e.g. \Article -> \WikiPage ). If we'd made the fields private and only expose them by protected getters we could even emit deprecation warnings if a subclass accesses them, thus making migration easier.

[1] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceFoundation/tree/bf63b4395efed92091db6dce25906ce196774d37/src/Hook
[2] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceFoundation/blob/bf63b4395efed92091db6dce25906ce196774d37/src/Hook/SkinTemplateOutputPageBeforeExec.php
[3] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceMultiUpload/blob/c588d9abdb70d87db101ecf130c65468e4889283/src/Hook/SkinTemplateOutputPageBeforeExec/AddDropZone.php
[4] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceMultiUpload/blob/c588d9abdb70d87db101ecf130c65468e4889283/extension.json#L36
[5] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceAuthors/blob/master/src/Hook/BeforePageDisplay/AddModules.php#L15
[6] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceFoundation/blob/bf63b4395efed92091db6dce25906ce196774d37/src/Hook/SkinTemplateOutputPageBeforeExec.php#L81-L87

CommunityTechBot raised the priority of this task from High to Needs Triage.Jul 3 2018, 1:58 AM
Legoktm renamed this task from Come up with a strategy for handling interface changes to Come up with a strategy for handling PHP interface changes.Oct 2 2018, 4:28 PM

This came up a while back in an internal meeting to revisit at some point. I'm not 100% sure whether the above is result of that already happening or whether there is something specific Daniel wanted us to do as a group.

Tagging on the committee workboard for now, instead of tracking as TODO on a document somewhere.

daniel added a comment.EditedMar 14 2019, 7:15 AM

This came up a while back in an internal meeting to revisit at some point. I'm not 100% sure whether the above is result of that already happening or whether there is something specific Daniel wanted us to do as a group.

Thanks for bringing this up again @Krinkle!

Since we had the discussion, I was in the position to create new "handler" type extension points again, and went for the abstract base class approach, following the thoughts we discussed during the meeting on this topic.

I think abstract base classes should be the recommendation for "handler" type extension points, and perhaps also for "service" type handler points. Providing base classes for hook handlers, as @Osnard suggests, seems a bit heavy weight for the general case, though I can see the appeal. I'd leave that aspect to be discussed on T212482: RFC: Evolve hook system to support "filters" and "actions" only.

The next could be drafting a best practices document for creating "handler" and "service" extension points (as opposed to hooks, which are covered by T212482). This is something TechCom can take on. That document should then be made official via an RFC.

(terminology for reference: "service" extension points are all services that can be replaced/redefined in the main service container (or any service container, really). "handler" extension points register code for handling a new "flavor" of some concept in a registry service - examples are ContentHandlers, SlotRoleHandlers, ApiModules, SpecialPages, etc).

@daniel Sounds good. Do you think this document should be an RFC-approved policy, or a guideline that you write/publish as-is and maintain virally as other guidelines?

Krinkle assigned this task to daniel.Mar 20 2019, 7:04 PM
Krinkle moved this task from Inbox to In progress on the TechCom board.

@daniel Sounds good. Do you think this document should be an RFC-approved policy, or a guideline that you write/publish as-is and maintain virally as other guidelines?

Something in between would be ideal. A TechCom endorsed guideline? Go go through an RFC, with the goal of improving and buy-in. I wouldn't shoot for approval of a fixed policy, though. Amending it should be a matter of talk page discussion.

Tgr added a comment.Mar 21 2019, 12:53 AM

At this point I'd say, any time you want to use an interface, you should probably use an abstract class instead. (Except maybe if that class is not going to be exposed externally, but then what's the point of an interface in the first place?)

Whether something should have an interface/abstract class in the first place is an orthogonal issue; I'd like to see something for hooks that provides type safety and change management, whether that's a versioned interface or an abstract trait or an event object. (I'd like to have some discussion on those options when I have the time, which might not be soon.) The extension interfaces RFC seems to be mostly about hook contracts (performance and caching and whatnot) which is an orthogonal concern from both.

At this point I'd say, any time you want to use an interface, you should probably use an abstract class instead. (Except maybe if that class is not going to be exposed externally, but then what's the point of an interface in the first place?)

I still see valid use cases for interfaces. I'd phrase it like this: Interfaces should be treated as internal to the module that contains them. Abstract base classes should be used to define extension points that can be implemented outside the module.

Exposing an interface for consumption can still be useful - e.g. by declaring a hook parameter to implement a specific interface, thereby limiting the hook handler's access to the "safe" part of the underlying implementation. Interfaces can also still be useful within the module, e.g. to facilitate transitioning from "smart" records to value objects. For instance, Title implementing LinkTarget, so code that accepts a LinkTarget can still take a Title - a subclass wouldn't work as soon as you try to have Title implement another new interface, such as PageIdentity.

Whether something should have an interface/abstract class in the first place is an orthogonal issue; I'd like to see something for hooks that provides type safety and change management, whether that's a versioned interface or an abstract trait or an event object. (I'd like to have some discussion on those options when I have the time, which might not be soon.)

@BPirkle recently proposed using anonymous classes as one-off interfaces for use in hook parameters. I like the idea, it creates a well defined narrow interface that is bound to the specific use case (i.e. the hook).

The extension interfaces RFC seems to be mostly about hook contracts (performance and caching and whatnot) which is an orthogonal concern from both.

That RFC has a very different focus, but it's not entirely optional: it calls for the hook parameters to be (mostly) immutable (or at least, to not allow mutation by the hook handler). This can be achieved by making some modifiable object implement a read-only interface, and using that interface (or anon class, see above) in the hook signature. So we are back to the topic of exposing interfaces, but only for consumption.

Michael added a subscriber: Michael.May 6 2019, 3:58 PM
Simetrical removed a subscriber: Simetrical.
Simetrical added a subscriber: Simetrical.

Is the sole purpose of using abstract classes to give some time for the deprecation process to happen?

My main issue with abstract classes is that it is very easy to drift from a general contract of methods and there are no language constraints to avoid:

  • adding properties to the abstract class because this is convenient
  • adding a constructor
  • having protected/private methods

I don't think that interfaces and abstract classes are mutually exclusive, I've often seen them being used for the same component:

  • provide an interface for consumption
  • provide an abstract base class usable by implementations for convenience

There are no guarantee that the extension will use your base class but this provides a cleaner a more flexible way to let extensions adhere to a contract:

  • the interface will always remain the "official" contract
  • if the abstract class starts to drift it's easier to ditch it by deprecating it and providing another one since it's only referenced by implementations
dcausse added a comment.EditedJul 23 2019, 5:03 PM

Sorry I came to this task because I face the problem where having an interface used for consumption would have helped a lot. My example is the SearchResult/SearchResultSet objects that a SearchEngine must produce. My problem is:

  • SearchResult and SearchResultSet are concrete classes used as type hint all over the place
  • I have to extend this type hierarchy but the constructor parameters expected no longer make sense

My plan so far:

  • Introduce an interface defining what is an SearchResult[Set] and use it as type hint everywhere (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/525101)
  • Add a minimal BaseSearchResult[Set] abstract base class based on my experience of what has been useful so far that SearchEngine implementation can extend for convenience
  • Eventually ditch the old SearchResult[Set] concrete classes
Tgr added a comment.Jul 23 2019, 5:08 PM

I have to extend this type hierarchy but the constructor parameters expected no longer make sense

If you extend a class, it shouldn't matter what the constructor parameters of the original class were. (Unless the class is constructed in code not controlled by you via some non-extensible mechanism, in which case that's your fundamental problem.)

I have to extend this type hierarchy but the constructor parameters expected no longer make sense

If you extend a class, it shouldn't matter what the constructor parameters of the original class were. (Unless the class is constructed in code not controlled by you via some non-extensible mechanism, in which case that's your fundamental problem.)

Do you mean avoiding to call the parent constructor?
While not calling the parent constructor is something allowed in PHP and may allow you to unlock some tricky situations I would not consider this to be a good practice.

Would a solution using a trait as an adapter to ease the transition be helpful to mitigate the constraints involved by only having a Base class?
Please see: P8791 for a short example
cons:

  • more code files

pros:

  • BC code isolated to a dedicated trait
  • can still implement the interface (and thus multiple ones) as long as you adhere to the BCTrait
  • call sites are type hinting with the interface
  • the interface remains a clear contract as opposed to a Base class that could rapidly become hard to read.

The constructor signature of the abstract base class would have to be treated as a public interface. Signature changes must be done in a backwards compatible way. PHP allows for polymorphic parameter lists via func_get_args. Not pretty, but doable.

Just like for the old methods that are kept for backwards compat and map to the new methods, the old constructor signature would be supported. Calls to deprecated methods and use of deprecated constructor signatures would trigger a deprecation warning. The B/C code would still be local to the abstract base class - which is exactly why we have it in the first place.

That being said: a base class that exists in lieu of a proper interface should probably have no members, and thus no need for a constructor. It's only once you start putting "shared utility code" into the base class that this need arises. Code sharing via subclassing is generally an anti-pattern anyway, and we should use composition instead (using traits or helper classes).

I feel that this guideline is very limiting esp the fact that it prevents to introduce short and isolated contract (e.g. things like LoggerAwareInterface).
For instance how this guideline would apply for things like DestructibleService?

daniel added a comment.EditedJul 26 2019, 11:39 AM

I feel that this guideline is very limiting esp the fact that it prevents to introduce short and isolated contract (e.g. things like ).
For instance how this guideline would apply for things like DestructibleService?

You are raising a good point, namely that fact that interfaces are indeed useful and should not be totally abandoned. That'as not the intention. The intention is that extension points should not be PHP interfaces, but rather be base classes.

E.g. an extension should not directly implement BlobStore, and perhaps BlobStore shouldn't be an interface but a base class. Or we should have AbstractBlobStore or BlobStoreBase for extensions to base their implementation on. That way, we could for instance as a bulk load interface to BlobStore, with a fallback implementation that just calls the regular getBlob() method for each blob, without breaking implementations.

A custom implementation of BlobStore could of course also implement DestructibleService and LoggerAwareInterface. These are not extension points in the sense that an extension would "register their DestructibleService". An extension would register their BlobStore, which might also be a DestructibleService.

In the context of refining our extension interface, extension points are either "hooks", or "services", or "handlers". This is about services and handlers (ContentHandlers, API modules, REST routes, etc).

I guess that line is blurry though, and I'm not quite sure what to call interfaces like DestructibleService and LoggerAwareInterface. They are indeed public and should be stable. They are not really extension points. Any good ideas what to call these kind of interfaces, so we can discuss them explicitly as a group?

debt triaged this task as Medium priority.Jul 30 2019, 5:26 PM
debt edited projects, added Discovery-Search; removed Discovery-Search (Current work).
debt moved this task from needs triage to watching / waiting on the Discovery-Search board.

I created a draft of a guideline for extension points, see https://www.mediawiki.org/wiki/Wikimedia_Technical_Committee/Extension_point_guidelines. It proposes to always use base classes as extension points. please comment there.

Krinkle renamed this task from Come up with a strategy for handling PHP interface changes to Strategy for handling PHP interface changes.Aug 21 2019, 7:42 PM
Izno added a subscriber: Izno.Sep 1 2019, 3:37 PM

After some discussion, it seems to me that this is primarily about defining what we consider a stable interface. I thnk we should amend the deprecation policy to be explicit about this. Draft here:
https://www.mediawiki.org/wiki/User:DKinzler_(WMF)/Stable_Interface

Krinkle renamed this task from Strategy for handling PHP interface changes to Strategy for PHP interface changes.Sep 18 2019, 7:50 PM
daniel moved this task from In progress to Backlog on the TechCom board.Sep 18 2019, 8:34 PM

Proposal needs an update, I plan to do this next week.

Just in case it helps the discussion: here is a list of all the PHP interfaces declared in core and how they are implemented in extensions. In the end there are very few and even fewer are the ones that could not be transformed to an abstract base class. I did not analyze inter-extension dependencies.

daniel updated the task description. (Show Details)Sep 19 2019, 2:49 PM

Just in case it helps the discussion: here is a list of all the PHP interfaces declared in core and how they are implemented in extensions. In the end there are very few and even fewer are the ones that could not be transformed to an abstract base class. I did not analyze inter-extension dependencies.

Thank you for making that list, that's very helpful!

daniel moved this task from Backlog to Inbox on the TechCom board.Sep 19 2019, 2:52 PM

Moved by to TechCom inbox. I have updated the task description to reflect the current state of the proposal. The draft for the stable interface policy can be found at https://www.mediawiki.org/wiki/Stable_Interface_Policy. If there are no objections, I think the draft can go on Last Call next week. Please put any comments or questions here rather than the talk page on the wiki. I'm more likely to see them here, and they are in context of the prior discussion.

Some comments/questions:

  • unstable vs internal: should unstable/experimental be considered as "use at your own risk" (or alike) instead of "do not use outside the module"
  • clarification of the scope of the annotation: I think it'd help clarity if every annotation had its allowed scope clarified (class, ctor, function). e.g. @newable does seem to only apply at the class level.

Overall I very much like this policy in general as it brings clarity without enforcing any particular design decision.
My sole concern could perhaps be that with many annotations there may be some ambiguous combination that could made.

Some comments/questions:

  • unstable vs internal: should unstable/experimental be considered as "use at your own risk" (or alike) instead of "do not use outside the module"

This essentially means the same thing. If it means "do not use outside the module" and you do it anyway, you won't get arrested. All it means is that you do it at your own risk.

  • clarification of the scope of the annotation: I think it'd help clarity if every annotation had its allowed scope clarified (class, ctor, function). e.g. @newable does seem to only apply at the class level.

Yes, that's correct. If you like, just add it to the draft :)

Overall I very much like this policy in general as it brings clarity without enforcing any particular design decision.
My sole concern could perhaps be that with many annotations there may be some ambiguous combination that could made.

True. I see no way to avoid that. Maybe a code sniffer rule?

daniel added a subscriber: Legoktm.Sep 25 2019, 7:36 PM

@Legoktm asked on the talk page:

What's the advantage of having a separate policy for this? Why not integrate the new stuff into the existing deprecation policy?

I answered there:

Because it's long, and defining what's stable is different from defining how to change stable things. The primary audience for the deprecation policy are core developers, while the primary audience for the stable interface policy are extension developers.

That said, I would also me OK with merging them into one. I'm a bit torn on which should contain which, though. Having them on the same level makes more sense to me.

Pinging @CCicalese_WMF, since this falls in the scope of the "narrowing the extension interface" initiative. We don't have a phab tag for that yet, right? Once the stable interface policy is established, we'll have to go through the core code and annotate classes and methods to indicate their stability, where it departs from the default. This would probably fall to the Platform Engineering. Could be a good intern project, though it's probably not terribly satisfying...

daniel renamed this task from Strategy for PHP interface changes to Establish stable interface policy for PHP code (was: strategy for PHP interface changes).Oct 1 2019, 9:49 AM
Krinkle added a comment.EditedOct 9 2019, 7:25 PM

Quick scan as of https://www.mediawiki.org/w/index.php?title=Stable_interface_policy&oldid=3451499:

  • The changes compared to last month LGTM.
  • Still a bit heavy imho on the number of (different) things being introduced and required. Maybe we can consolidate some of the proposed annotations where their outcome is effectively the same and leave intent to something communicated socially, in the commit message, or elsewhere. Or maybe inline after the annotation. As (perhaps bad) example: instead of @extensible and @stable we could have @stable and @stable extensible instead.
  • I found it confusing that the first section of this "core policy" is the non-policy recommendation to extensions. Might be better re-ordered in some way, or perhaps generalised to only cover the "core policy" of it, and maybe a note elsewhere on the page (or in the coding conventions) that extensions are encouraged to follow X.
  • From talk page:
    • I'd like us to consider (not yet sure if it's better) proposing this as an amendment to the "Deprecation policy" instead of as enacting an entirely new policy. Maybe the draft could incorporate the existing page and give it a better name to cover both, as a single policy.
    • It would be useful to have a shorter summary that will than explain the difference (e.g. what we added/changed), so that participants here can read that instead of the whole page, which would help them decide between providing feedback on the nominal changes of the policy vs the actual policy page as a whole.

I'm thinking about the generated documentation that may now be confusing to newcomers / people who haven't read this policy yet. For example, if you have two public methods, and one of them is annotated with @stable while the other is not. I would read the documentation and use any public method and expect it to be stable unless something jumped out at me telling me not to make this assumption. So, ideally, docs would color/explain both annotated methods and public methods that are not annotated.

Krinkle moved this task from Inbox to In progress on the TechCom board.Oct 9 2019, 10:00 PM
daniel added a comment.EditedOct 10 2019, 1:49 PM

I'm thinking about the generated documentation that may now be confusing to newcomers / people who haven't read this policy yet. For example, if you have two public methods, and one of them is annotated with @stable while the other is not. I would read the documentation and use any public method and expect it to be stable unless something jumped out at me telling me not to make this assumption. So, ideally, docs would color/explain both annotated methods and public methods that are not annotated.

Per the policy, public methods are considered stable (safe to call) per default. However, public constructors are not considered stable. And public methods are not automatically considered fixed (safe to override).

If I understand you right, you would like an explicit notice on the generated documentation for all constructors that state that they are not safe to call (unless marked as stable, or the class being marked newable).

I don't know how hard that would be do achieve. Also, seeing the same notice on most constructors make result in "banner blindness"...

daniel updated the task description. (Show Details)Oct 10 2019, 3:11 PM

Per the policy, public methods are considered stable (safe to call) per default. However, public constructors are not considered stable. And public methods are not automatically considered fixed (safe to override).

oh ok, it sounded from the document like some public methods are unstable and could be marked as such. It's weird that public constructors are not stable, but as long as it's defined somewhere I think it's fine to have conventions like that. I don't think overrides would be confusing so that's fine, as long as all public methods are stable.

If I understand you right, you would like an explicit notice on the generated documentation for all constructors that state that they are not safe to call (unless marked as stable, or the class being marked newable).

I don't know how hard that would be do achieve. Also, seeing the same notice on most constructors make result in "banner blindness"...

agreed, so it's fine if there's a good and simple convention. The point I'm trying to make is basically about the principle of least surprise. As long as the annotations couldn't be put on methods in a way that would surprise/astonish a normal developer, then great.

If I understand you right, you would like an explicit notice on the generated documentation for all constructors that state that they are not safe to call (unless marked as stable, or the class being marked newable).

I don't know how hard that would be do achieve. Also, seeing the same notice on most constructors make result in "banner blindness"...

Ideally, it would be good to have documentation of everything not meant for public consumption to be hidden/collapsed/grayed out/in a separate section/otherwise clearly indicated as not being for public use. This wouldn't result in banner blindness, because there'd be no banner, it just wouldn't appear with the things a random extension developer is supposed to be looking at.

oh ok, it sounded from the document like some public methods are unstable and could be marked as such.

They can be. This would probably be because we're using the methods in other classes before we've finalized the interface.

It's weird that public constructors are not stable, but as long as it's defined somewhere I think it's fine to have conventions like that.

It's because we're porting everything to dependency injection, so each class' dependencies are injected in its constructor. If a new dependency is added, we need to add a new required argument to the constructor, which will break callers. Normally the constructors of such classes should only be called by MediaWikiServices.

It occurs to me that we need to define rules for changes in stability similar to rules for deprecation and removal. If I want to mark a public interface as internal when it wasn't officially marked as such in the past, does that need a mention in release notes? If so, where -- the deprecation section or somewhere else? Are there any rules for how soon I can remove a method (or otherwise break callers) after declaring it unstable? Is there going to be any equivalent of hard deprecation for declaring things unstable?

It could be we don't have to worry about all these cases, because declaring stable interfaces as internal or unstable might not be common enough to warrant worrying about (as opposed to deprecation). But it should be thought about.

Pinging @CCicalese_WMF, since this falls in the scope of the "narrowing the extension interface" initiative. We don't have a phab tag for that yet, right? Once the stable interface policy is established, we'll have to go through the core code and annotate classes and methods to indicate their stability, where it departs from the default. This would probably fall to the Platform Engineering. Could be a good intern project, though it's probably not terribly satisfying...

We do not have a phab tag for that yet, but I'll add this to our Future Initiatives list.

oh ok, it sounded from the document like some public methods are unstable and could be marked as such.

They can, but this should be rare. That could be the case e.g. when introducing a new method that needs to be public because it's called from elsewhere in the module, but isn't (yet) stable for use by other modules.

As long as the annotations couldn't be put on methods in a way that would surprise/astonish a normal developer, then great.

The point of annotations is to mark deviation from the norm... Whether "the constructor is public, but not stable for use outside the module" is surprising depends on the developer. It would have surprised me ten years ago I guess. Now I'm used to think in terms of CI, so it seems obvious.

daniel added a comment.EditedDec 18 2019, 11:32 AM

It occurs to me that we need to define rules for changes in stability similar to rules for deprecation and removal. If I want to mark a public interface as internal when it wasn't officially marked as such in the past, does that need a mention in release notes? If so, where -- the deprecation section or somewhere else? Are there any rules for how soon I can remove a method (or otherwise break callers) after declaring it unstable? Is there going to be any equivalent of hard deprecation for declaring things unstable?

Good point. Declaring something unstable or internal after the fact is kind of like changing something from public to private. It needs a "deprecation" period, but I don't see a good way to implement hard deprecation for this - you'd have to detect whether the current caller is an "allowed" caller.

The cleanest solution in such a case is to make a new method and deprecate the old method. The downside is that this means inventing new names that are nearly the same as the old name.

Some notes from @daniel and myself during last week's TechCom meeting:

What about this proposal:

  • All interfaces should be marked as @internal by default.
  • Generally extensions should extend abstract base classes instead, so that changes in signatures and new methods are not breaking by default.
  • For rare exceptions, the interface may be explicitly marked as @stable.

Cost of having the interface at all when it is @internal:

  • Extra thing to maintain.
  • Requires devs think about abstract and interface separately.

Benefit:

  • Makes for easier mocks in PHPUnit by allowing use of “final” in the base class.
  • Avoids leaking public methods for internal use into the mock.
  • Enables using a narrower interface as type-hint.
  • Clean interface/docs file for improved developer UX.
  • Allows abstract class to implement multiple interfaces.

Three use cases:

  1. Extension point default: Recommendation is @stable abstract base class. Interface not needed. If we want to use an interface within core, it can exist and be marked @internal. If authors wish to maintain both as public and non-breaking points they can mark it @stable, but this is not recommended. Pre-existing can either be deprecated to match the recommendation or be kept as stable for compat, author's choice.
  2. Extension point opt-in: If authors wish to vary from this recommendation by providing an interface as extension point (without abstract class) then the interface in question must be marked @stable.
  3. Narrow interfaces for services we don’t recommend implementing directly.

For example, “ReadableRevisionStore” and “WritableRevisionStore”. These interfaces exist to aid in keeping code separated and predictable in behaviour. Bu are only meant for use in type hints, not implemented directly. Actual implementation should use an abstract base class that implements both, which allows breaking changes to the interface without deprecation so long the abstract class covers it.

Open question: For this use case the abstract base class is @stable, but the interface would be “@stable but not allowed for implementation” - How do we express such a contract?
A new annotation instead of @stable?
A additive keyword like “@stable hint-only” or “@stable non-extendable”
Something else?

TheDJ added a subscriber: TheDJ.Feb 24 2020, 1:18 PM
daniel updated the task description. (Show Details)Feb 26 2020, 10:09 PM
daniel updated the task description. (Show Details)

Per today's TechCom meeting, the Stable Interface Policy proposed on https://www.mediawiki.org/wiki/Stable_Interface_Policy is going on Last Call. If no pertinent concerns remain unaddressed by March 11, the policy will be adopted as proposed.

daniel updated the task description. (Show Details)Feb 28 2020, 10:37 AM
Krinkle renamed this task from Establish stable interface policy for PHP code (was: strategy for PHP interface changes) to RFC: Establish stable interface policy for PHP code (was: strategy for PHP interface changes).Mar 4 2020, 5:03 PM
Krinkle renamed this task from RFC: Establish stable interface policy for PHP code (was: strategy for PHP interface changes) to RFC: Establish stable interface policy for PHP code.
saper added a subscriber: saper.Mar 5 2020, 4:08 PM

Per yesterday's TechCom meeting, this RFC has been approved. The draft is now official policy.

daniel closed this task as Resolved.Mar 12 2020, 11:31 AM