⚓ T193613 RFC: Establish stable interface policy for PHP code

⚓ T193613 RFC: Establish stable interface policy for PHP code
Page Menu
Phabricator
Create Task
Maniphest
T193613
RFC: Establish stable interface policy for PHP code
Closed, Resolved
Public
Actions
Edit Task
Edit Related Tasks...
Create Subtask
Edit Parent Tasks
Edit Subtasks
Merge Duplicates In
Close As Duplicate
Edit Related Objects...
Edit Commits
Edit Mocks
Mute Notifications
Protect as security issue
Assigned To
daniel
Authored By
Tgr
May 2 2018, 10:18 AM
2018-05-02 10:18:09 (UTC+0)
Tags
MediaWiki-General
(Backlog)
Platform Engineering
(Tracking/Watching)
TechCom-RFC (TechCom-RFC-Closed)
(Implemented)
Discovery-Search (Current work)
(Incoming)
Referenced Files
None
Subscribers
Addshore
Aklapper
BPirkle
CCicalese_WMF
daniel
dcausse
Izno
View All 18 Subscribers
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'
);
class
Core1
public
function
$a
$b
null
{}
class
Extension1
extends
Core1
public
function
$a
{}
// will raise a warning but still work
class
Core2
public
function
$a
{}
public
function
f2
$b
{}
class
Extension2
extends
Core2
public
function
$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
$a
$b
null
);
class
Extension3
implements
Core3
public
function
$a
{}
// fatal error
interface
Core4
public
function
$a
);
public
function
f2
$b
);
class
Extension4
implements
Core4
public
function
$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:
Adopt the
Stable Interface Policy as drafted at
. 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
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
Related Objects
Mentions
Mentioned In
T268326: RFC: Amendment to the Stable interface policy (November 2020)
T267085: Clarify deprecation of method overrides in the stable interface policy
T255803: RFC: Amendment to the Stable interface policy, June 2020
T247836: Update UnrecognizedAnnotation following new stable interface policy
T245710: LocalFileDeleteBatch accepts an optional user, falls back to $wgUser
T241180: RFC: Adopt a modern JavaScript framework for use with MediaWiki
T234316: upgrade to upcoming wdio-mediawiki version
T203410: Provide a narrow interface for code that needs to wait for DB replication lag
T221177: REST route handler extension interface RFC
T194925: Public TechCom Meeting
P7112 Interface versioning example for T193613
T194038: Introduce ContentHandler::getSecondaryDataUpdates to replace Content::getSecondaryDataUpdates
Mentioned Here
P8791 Use trait as an adapter to ease interface changes
T212482: RFC: Evolve hook system to support "filters" and "actions" only
P7111 TechCom RfC meeting 2018-05-10
P7112 Interface versioning example for T193613
Event Timeline
There are a very large number of changes, so older changes are hidden.
Show Older Changes
CommunityTechBot
raised the priority of this task from
High
to
Needs Triage
Jul 3 2018, 1:58 AM
2018-07-03 01:58:23 (UTC+0)
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
2018-10-02 16:28:54 (UTC+0)
Krinkle
added a project:
TechCom
Mar 14 2019, 12:27 AM
2019-03-14 00:27:04 (UTC+0)
Krinkle
subscribed.
Mar 14 2019, 12:29 AM
2019-03-14 00:29:24 (UTC+0)
Comment Actions
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.
Edited
Mar 14 2019, 7:15 AM
2019-03-14 07:15:21 (UTC+0)
Comment Actions
In
T193613#5023037
@Krinkle
wrote:
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).
Krinkle
added a comment.
Mar 20 2019, 7:02 PM
2019-03-20 19:02:30 (UTC+0)
Comment Actions
@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
2019-03-20 19:04:22 (UTC+0)
Krinkle
moved this task from
Inbox
to
In progress
on the
TechCom
board.
daniel
added a comment.
Mar 20 2019, 7:53 PM
2019-03-20 19:53:07 (UTC+0)
Comment Actions
In
T193613#5041226
@Krinkle
wrote:
@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
2019-03-21 00:53:19 (UTC+0)
Comment Actions
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.
daniel
added a subscriber:
BPirkle
Mar 21 2019, 11:33 AM
2019-03-21 11:33:51 (UTC+0)
Comment Actions
In
T193613#5042462
@Tgr
wrote:
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.
daniel
added a comment.
Mar 21 2019, 10:01 PM
2019-03-21 22:01:53 (UTC+0)
Comment Actions
First draft:
Please comment.
Tgr
mentioned this in
T221177: REST route handler extension interface RFC
Apr 24 2019, 9:20 PM
2019-04-24 21:20:15 (UTC+0)
Michael
subscribed.
May 6 2019, 3:58 PM
2019-05-06 15:58:51 (UTC+0)
Simetrical
subscribed.
May 7 2019, 10:39 AM
2019-05-07 10:39:50 (UTC+0)
Simetrical
unsubscribed.
Simetrical
subscribed.
dcausse
subscribed.
Jul 23 2019, 4:09 PM
2019-07-23 16:09:44 (UTC+0)
Comment Actions
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.
Edited
Jul 23 2019, 5:03 PM
2019-07-23 17:03:29 (UTC+0)
Comment Actions
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 (
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
2019-07-23 17:08:21 (UTC+0)
Comment Actions
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.)
debt
added a project:
Discovery-Search (Current work)
Jul 23 2019, 5:41 PM
2019-07-23 17:41:29 (UTC+0)
Tgr
mentioned this in
T203410: Provide a narrow interface for code that needs to wait for DB replication lag
Jul 23 2019, 6:41 PM
2019-07-23 18:41:47 (UTC+0)
dcausse
added a comment.
Jul 23 2019, 7:18 PM
2019-07-23 19:18:44 (UTC+0)
Comment Actions
In
T193613#5357929
@Tgr
wrote:
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.
dcausse
added a comment.
Jul 24 2019, 9:23 AM
2019-07-24 09:23:28 (UTC+0)
Comment Actions
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.
daniel
added a comment.
Jul 24 2019, 12:21 PM
2019-07-24 12:21:16 (UTC+0)
Comment Actions
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).
dcausse
added a comment.
Jul 24 2019, 1:23 PM
2019-07-24 13:23:03 (UTC+0)
Comment Actions
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.
Edited
Jul 26 2019, 11:39 AM
2019-07-26 11:39:15 (UTC+0)
Comment Actions
In
T193613#5361165
@dcausse
wrote:
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
2019-07-30 17:26:09 (UTC+0)
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.
Mholloway
unsubscribed.
Jul 30 2019, 5:27 PM
2019-07-30 17:27:39 (UTC+0)
daniel
added a comment.
Aug 15 2019, 12:50 PM
2019-08-15 12:50:32 (UTC+0)
Comment Actions
I created a draft of a guideline for extension points, see
. 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
2019-08-21 19:42:33 (UTC+0)
daniel
moved this task from
Under discussion
to
P1: Define
on the
TechCom-RFC
board.
Aug 27 2019, 2:34 PM
2019-08-27 14:34:55 (UTC+0)
Izno
subscribed.
Sep 1 2019, 3:37 PM
2019-09-01 15:37:44 (UTC+0)
daniel
added a comment.
Sep 11 2019, 8:22 PM
2019-09-11 20:22:43 (UTC+0)
Comment Actions
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:
Krinkle
renamed this task from
Strategy for handling PHP interface changes
to
Strategy for PHP interface changes
Sep 18 2019, 7:50 PM
2019-09-18 19:50:09 (UTC+0)
daniel
moved this task from
In progress
to
Backlog
on the
TechCom
board.
Sep 18 2019, 8:34 PM
2019-09-18 20:34:52 (UTC+0)
Comment Actions
Proposal needs an update, I plan to do this next week.
dcausse
added a comment.
Sep 19 2019, 9:43 AM
2019-09-19 09:43:58 (UTC+0)
Comment Actions
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
2019-09-19 14:49:20 (UTC+0)
Comment Actions
In
T193613#5505604
@dcausse
wrote:
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
2019-09-19 14:52:33 (UTC+0)
Comment Actions
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
. 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.
dcausse
added a comment.
Sep 19 2019, 4:23 PM
2019-09-19 16:23:00 (UTC+0)
Comment Actions
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.
daniel
added a comment.
Sep 25 2019, 7:34 PM
2019-09-25 19:34:17 (UTC+0)
Comment Actions
In
T193613#5506792
@dcausse
wrote:
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
2019-09-25 19:36:14 (UTC+0)
Comment Actions
@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.
daniel
moved this task from
P1: Define
to
Under discussion
on the
TechCom-RFC
board.
Sep 25 2019, 8:42 PM
2019-09-25 20:42:30 (UTC+0)
daniel
added a subscriber:
CCicalese_WMF
Oct 1 2019, 9:48 AM
2019-10-01 09:48:01 (UTC+0)
Comment Actions
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
2019-10-01 09:49:28 (UTC+0)
Michael
mentioned this in
T234316: upgrade to upcoming wdio-mediawiki version
Oct 1 2019, 11:00 AM
2019-10-01 11:00:08 (UTC+0)
Krinkle
added a comment.
Edited
Oct 9 2019, 7:25 PM
2019-10-09 19:25:55 (UTC+0)
Comment Actions
Quick scan as of
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.
Milimetric
subscribed.
Oct 9 2019, 9:48 PM
2019-10-09 21:48:02 (UTC+0)
Comment Actions
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
2019-10-09 22:00:12 (UTC+0)
daniel
added a comment.
Edited
Oct 10 2019, 1:49 PM
2019-10-10 13:49:19 (UTC+0)
Comment Actions
In
T193613#5561446
@Milimetric
wrote:
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
2019-10-10 15:11:09 (UTC+0)
Milimetric
added a comment.
Oct 22 2019, 9:33 PM
2019-10-22 21:33:48 (UTC+0)
Comment Actions
In
T193613#5562950
@daniel
wrote:
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.
Simetrical
added a comment.
Oct 23 2019, 8:56 AM
2019-10-23 08:56:50 (UTC+0)
Comment Actions
In
T193613#5562950
@daniel
wrote:
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.
In
T193613#5596889
@Milimetric
wrote:
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.
Simetrical
added a comment.
Oct 23 2019, 2:36 PM
2019-10-23 14:36:04 (UTC+0)
Comment Actions
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.
CCicalese_WMF
added a project:
Platform Engineering
Nov 6 2019, 7:01 PM
2019-11-06 19:01:12 (UTC+0)
Comment Actions
In
T193613#5537074
@daniel
wrote:
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.
CCicalese_WMF
moved this task from
Inbox
to
Future Initiatives/Small Projects
on the
Platform Engineering
board.
Nov 6 2019, 7:01 PM
2019-11-06 19:01:28 (UTC+0)
daniel
added a comment.
Dec 18 2019, 11:28 AM
2019-12-18 11:28:27 (UTC+0)
Comment Actions
In
T193613#5596889
@Milimetric
wrote:
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.
Edited
Dec 18 2019, 11:32 AM
2019-12-18 11:32:20 (UTC+0)
Comment Actions
In
T193613#5599000
@Simetrical
wrote:
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.
Krinkle
added a comment.
Feb 12 2020, 5:16 PM
2020-02-12 17:16:48 (UTC+0)
Comment Actions
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:
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.
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
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?
kchapman
removed a project:
TechCom
Feb 12 2020, 9:20 PM
2020-02-12 21:20:10 (UTC+0)
TheDJ
subscribed.
Feb 24 2020, 1:18 PM
2020-02-24 13:18:18 (UTC+0)
daniel
added a comment.
Feb 26 2020, 8:32 PM
2020-02-26 20:32:34 (UTC+0)
Comment Actions
Updated draft at
daniel
updated the task description.
(Show Details)
Feb 26 2020, 10:09 PM
2020-02-26 22:09:26 (UTC+0)
daniel
updated the task description.
(Show Details)
daniel
moved this task from
Future Initiatives/Small Projects
to
Tracking/Watching
on the
Platform Engineering
board.
Feb 26 2020, 10:12 PM
2020-02-26 22:12:08 (UTC+0)
Comment Actions
Per today's TechCom meeting, the Stable Interface Policy proposed on
is going on Last Call. If no pertinent concerns remain unaddressed by March 11, the policy will be adopted as proposed.
daniel
moved this task from
Under discussion
to
P5: Last Call
on the
TechCom-RFC
board.
Feb 26 2020, 10:12 PM
2020-02-26 22:12:18 (UTC+0)
daniel
mentioned this in
T241180: RFC: Adopt a modern JavaScript framework for use with MediaWiki
Feb 27 2020, 10:47 AM
2020-02-27 10:47:51 (UTC+0)
daniel
updated the task description.
(Show Details)
Feb 28 2020, 10:37 AM
2020-02-28 10:37:01 (UTC+0)
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
2020-03-04 17:03:34 (UTC+0)
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
Addshore
subscribed.
Mar 4 2020, 6:17 PM
2020-03-04 18:17:23 (UTC+0)
saper
subscribed.
Mar 5 2020, 4:08 PM
2020-03-05 16:08:38 (UTC+0)
daniel
edited projects, added
TechCom-RFC (TechCom-RFC-Closed)
; removed
TechCom-RFC
Mar 12 2020, 11:31 AM
2020-03-12 11:31:11 (UTC+0)
Comment Actions
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
2020-03-12 11:31:20 (UTC+0)
Southparkfan
awarded a token.
Mar 13 2020, 12:43 PM
2020-03-13 12:43:17 (UTC+0)
DannyS712
mentioned this in
T245710: LocalFileDeleteBatch accepts an optional user, falls back to $wgUser
Mar 13 2020, 6:24 PM
2020-03-13 18:24:12 (UTC+0)
DannyS712
mentioned this in
T247836: Update UnrecognizedAnnotation following new stable interface policy
Mar 17 2020, 3:20 PM
2020-03-17 15:20:56 (UTC+0)
kchapman
moved this task from
Untriaged
to
Approved
on the
TechCom-RFC (TechCom-RFC-Closed)
board.
Mar 18 2020, 8:31 PM
2020-03-18 20:31:03 (UTC+0)
Krinkle
mentioned this in
T255803: RFC: Amendment to the Stable interface policy, June 2020
Jun 18 2020, 5:45 PM
2020-06-18 17:45:20 (UTC+0)
Demian
mentioned this in
T267085: Clarify deprecation of method overrides in the stable interface policy
Nov 3 2020, 4:50 AM
2020-11-03 04:50:51 (UTC+0)
daniel
mentioned this in
T268326: RFC: Amendment to the Stable interface policy (November 2020)
Nov 23 2020, 12:33 PM
2020-11-23 12:33:58 (UTC+0)
Krinkle
moved this task from
Approved
to
Implemented
on the
TechCom-RFC (TechCom-RFC-Closed)
board.
Dec 10 2020, 5:15 AM
2020-12-10 05:15:26 (UTC+0)
Gehel
edited projects, added
Discovery-Search (Current work)
; removed
Discovery-Search
Feb 12 2025, 9:35 AM
2025-02-12 09:35:20 (UTC+0)
Log In to Comment
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct.
Wikimedia Foundation
Code of Conduct
Disclaimer
CC-BY-SA
GPL
Credits