[policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release

If we type hint against the entity class, that explicitly prevents someone implementing the interface directly, which is a lot stricter than just saying we'll allow methods to be added on those interfaces that have an associated base class.

How does this affect UI?

How does this affect UI?

I hope it doesn't do at all.

For core I totally agree that having interfaces on entities are not only pointless, but they cause harm. For contrib though I can see the point in still having interface, but not an interface which extends EntityInterface/ContentEntityInterface. This way you could provide different implementations of storing stuff, like commerce might wants to do at some point.

On the original issue which introduced them: #1391694: Use type-hinting for entity-parameters people have argued a lot with coding standards, well done. There have been a couple of points, that there might be usecases for implementing the same thing in different ways. https://www.drupal.org/node/1391694/revisions/5621441/view

For myself, I can't really understand how anything as fundamental to Drupal as the entity API could reasonably be marked "internal" without making "internal" essentially meaningless and not actually following the spirit of semver.

If the goal of the issue is to "Determine how to add methods to entities while preserving BC" or something, can we make the scope that instead of a specific proposed solution?

Wouldn't the preferred way to solve this be to add methods to new interfaces, and deprecate the old methods if we want to change them? Or if that doesn't work for some reason, code samples showing the problem would be helpful.

@xjm: See the link in #2 to my comment and the discussion there. We could, but by 8.4 or so, we'd have entity types with lots and lots of additional, pseudo-optional interfaces and we'd have complicated code that needs to deal with the (very theoretical) case of entities that don't implement them.

The reason for marking the interfaces as internal/deprecated is to make the classes themself the API. And it's absolutely no problem to add new methods to base classes (unless you happen to define the same in subclasses, but we already defined that's OK for minor releases I think).

In general, for many classes that are are basically value classes, we shouldn't have added interfaces in the first place. FormState is another good example for that, there are issues that are complicated by discussions on how to add methods to it. But there's really no reason to write your own completely different implementation of. It's the same for Node. If you're doing something crazy (like file_entity is doing for for the file entity), then you might want to subclass it. But provide a completely different class that doesn't extend from Node?

In my opinion, that's basically what we're after. Requiring that any replacement for Node must extend from the default class so we can add new methods. And if you don't, then your code might break in a minor release. Deprecating the interfaces would be one way to do it.

I guess that using (e.g.) Node::load() as we do in a lot of places also means that swapping out a different implementation won't necessarily go as expected anyway.

So is the proposal only to mark interfaces for specific entity types internal? (NodeInterface, CommentInterface, etc.). That's a lot less than "all entity interfaces" which to me would mean everything from EntityInterface and RevisionableInterface to EntityAccessControlHandlerInterface, etc.

I took this to mean "the interfaces that correlate 1:1 with an entity type". So yes to NodeInterface, not to RevisionableInterface.

I'm +1 to that

I guess that using (e.g.) Node::load() as we do in a lot of places also means that swapping out a different implementation won't necessarily go as expected anyway.

This particular bit actually works. We have a concept of an original class, which is taken into account, even if you override the actual implementation.

In general I totally agree with the general idea of the issue. With our services model we move more and more into a direction of separating logic and data (which is a bit different to normal OOP), but more like other constructs in the world of software design. When you separate the logic and the data, the data is just data, its not something you want to contain logic anymore.

In this sense having those interfaces as internal sounds like a great idea.

Yes, I'm +1 on the concept as well.

However, as I said above (#11), IMHO @internal sends the wrong message. We definitely commit to any methods we have, we just want to add more. @internal means we could change them. And e.g. PhpStorm will strike them through like a deprecated.

So we could just as well go with @deprecated, because that's really what we're actually doing according to the description behind it.

Personally, I still the easiest solution would be just another chapter in #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation that describes those special rules :)

I have to agree that NodeInterface and friends, in hindsight, were a mistake, and agree with nudging people away from them. So +1 on adding @deprecated to those, with appropriate explanation text.

However, we should make sure that anywhere we're doing introspection currently, such as param converters et al, would not get cranky if we switch parameter types from NodeInterface to Node. If we're deprecating NodeInterface and company then we'll need to remove usage of them from core, which means making sure nothing breaks if we do so.

There's a big difference between the following two cases assuming someone has gone to the trouble of replacing Node without subclassing:

1. We add a method to Node and you didn't subclass it, so you have to add that method to your replacement class.

2. We removed all NodeInterface type hints and replace them with Node, and you didn't subclass it, and now your class has inherit from Node or it won't work.

Just want to be clear about what the choices are. If the theoretical module in #2 doesn't exist, then changing the type hints also prevents anyone from creating one in the future, so over time it's less likely to cause nasty surprises but it could still do so now.

On @internal vs. @deprecated, it'd be possible to do something like @internal the interface (with an explanation) but then individually @api the individual methods.

But then we could also @deprecate the interface, and @api the individual methods on Node.

I'm slowly leaning towards @deprecated as long as changing all those type hints doesn't actually break someone in practice.

I confirm this won't affect Multiversion.

I also hope it won't affect any contrib, because I can't get my head around why (or how without things breaking) you would implement an interface like NodeInterface.

I feel for all the code (I expect some of mine) that has lines like if ($entity instanceof NodeInterface), but hey, PHPStorm will flag it, and it can easily be updated.

The "1-1" clarification makes a big difference, and maybe we should make this a pattern, by updating Drupal Console and documentation, that content entities should implement interfaces like ContentEntityInterface, EntityChangedInterface, EntityOwnerInterface, rather than using their own interface (which often ends up being empty).

+1 to @deprecated.

Also, would a patch for this go as far is updating the use of these deprecated interfaces?

For example

diff --git a/core/modules/user/src/EntityOwnerInterface.php b/core/modules/user/src/EntityOwnerInterface.php
index ec9aa43..8079f66 100644
--- a/core/modules/user/src/EntityOwnerInterface.php
+++ b/core/modules/user/src/EntityOwnerInterface.php
@@ -7,6 +7,8 @@
 
 namespace Drupal\user;
 
+use Drupal\user\Entity\User;
+
 /**
  * Defines a common interface for entities that have an owner.
  *
@@ -20,7 +22,7 @@
   /**
    * Returns the entity owner's user entity.
    *
-   * @return \Drupal\user\UserInterface
+   * @return \Drupal\user\Entity\User
    *   The owner user entity.
    */
   public function getOwner();
@@ -28,12 +30,12 @@ public function getOwner();
   /**
    * Sets the entity owner's user entity.
    *
-   * @param \Drupal\user\UserInterface $account
+   * @param \Drupal\user\Entity\User $account
    *   The owner user entity.
    *
    * @return $this
    */
-  public function setOwner(UserInterface $account);
+  public function setOwner(User $account);
 
   /**
    * Returns the entity owner's user ID.

I'm fine with removing usages/type hints in a follow-up. This isn't a case where we could get tripped up by not having a replacement, we already know what the replacement type hint is.

I'm okay with #26 as well.

Per #27 and #28.

This seems like a really big change to be making and it would completely rule out the replacement of an entity type's class with one that doesn't extend the original one. Not saying this is a common use case, but it should be mentioned as a consequence here.

Core also seems to have a track record of deprecating functions by turning them into wrappers for the new function on the base class. (This is by looking at the code, so don't hold this observation against me.) But doing so would still break contrib for those who implemented the interface directly, so that may not be the solution either.

So it does seem like this is the right way forward, but it is a really big change nonetheless.

I understand the reasoning, but my gut doesn't like this.

It tells contrib to stop implementing interfaces for their entity types, and rely on entity classes alone. Entity API is pretty messy, but it feels like this change would make it even messier. Right now reading an entity interface tells me which kind of entity it is (content/config), which features it supports (owner, changed, etc) and gives me an overview of important methods without the noise of implementation.
Plus, it feels like we're making a leap from "this interface might change" to "it's impossible to swap this implementation". Having to implement two new methods every 6 months does not equal impossible.

So, I agree with catch's initial thoughts in #21.

This seems like a really big change to be making and it would completely rule out the replacement of an entity type's class with one that doesn't extend the original one. Not saying this is a common use case, but it should be mentioned as a consequence here.

Yes, that's exactly what this issue is about. Disallowing that.

And yes, *if* someone already has done something like this, then it is a big change for them. However, the only immediately impact is that type hints won't work anymore for them and if we add more methods in the future, then their code will break. Which it would too if we follow my initial suggestion of allowing additional methods on the interface (it would break immediately, with a clear message, though).

But I'm not aware of anyone doing that at the moment nor do I know a valid use case for doing that. config entities are tied to storage and their schema/yaml definitions anyway and content entities just the same to ther field definitions. Replacing with a subclass is a rare enough use case (I do it in file_entity, to mess with bundle handling), that's currently the only example/use case I know of.

Also, we don't technically disallow using a class, you're just on your own and your code might break in minor releases. Just like when you use @internal code.

We have +1 from @xjm and @catch, I'm OK with this too, as is everyone else so far. Lets do this.

Per #13

If the goal of the issue is to "Determine how to add methods to entities while preserving BC" or something, can we make the scope that instead of a specific proposed solution?

The answer should not be removing interfaces completely. While it might be a burden to replace the Node entity class with a custom implementation, it still could technically be done because we're expecting interface implementations.

It seems a discussion needs to be made on what is allowed for interface modification during minor releases. I would not expect a patch release to change an interface, however, I could understand a minor release adding a method, then major removing methods. While the addition of a method implements a breaking change, it

It just seems drastic to go "well, shoot, I didn't like that window in my house, so burn the wall down."

@bojanz, sorry, confused by your response. Could you clarify?

I understand the reasoning, but my gut doesn't like this.

It tells contrib to stop implementing interfaces for their entity types, and rely on entity classes alone. Entity API is pretty messy, but it feels like this change would make it even messier. Right now reading an entity interface tells me which kind of entity it is (content/config), which features it supports (owner, changed, etc) and gives me an overview of important methods without the noise of implementation.

I don't think any of that would be going away? Node would still implement ContentEntityInterface, EntityChangedInterface, EntityOwnerInterface.

So, I agree with catch's initial thoughts in #21.

But @catch's initial thoughts in #21 are what led to the patch in its current form?

We're currently discussing a different idea in skype (someone should post a log..), so taking back my RTBC for now.

We had a long IRC discussion around this (alexpott, catch, berdir and myself).
I got around to understanding the reasoning behind this issue. However, I think that we're trying to solve the wrong problem.

Fact is, core has public and non-public interfaces.
Public are the interfaces that you need to implement in order to plug into the system (define an entity, block, theme negotiator).
That includes plugin interfaces, tagged service interfaces. These interfaces need to stay static no matter what.

The other kind are non-public interfaces. These are the ones that Core has to make its own code cleaner and easier to reason about.
NodeInterface, FormStateInterface, general service interfaces. As we add features, we need to be able to expand these interfaces between between minor releases (8.1, 8.2, 8.3). That means adding new methods, not changing old ones (they can just be deprecated).
That leaves us with two options:
a) Significantly reduce the amount of non-public interfaces in core.
That's what this issue tries to do. Kill entity-specific interfaces like NodeInterface completely, tell contrib never to make them.
A very big policy change for 8.1.
b) Document that we reserve the right to expand non-public interfaces in minor releases.

I think b) is a much more sensible strategy. We tag our public interfaces with @api and enforce no-changes. The non-public interfaces are open to new methods.
People swapping an entity class or a core service can expect minimal breakage if they're not extending the classes they are replacing.

This approach had support on IRC, and gives us a lot more bang for the buck future-wise, so I'm setting the issue back to "needs review".

I really like what has been summarized in #36

+1 to #36.b.

Re #39, the end of that article also makes a distinction between "published" and "immutable", implying that interfaces for which the only allowed change is method additions can still be considered "published", but not "immutable". Am I understanding right that all Drupal interfaces are effectively "published" (in the sense that we will not break contrib/custom callers ever, except if absolutely necessary to fix a critical bug), but that ones marked @api are additionally "immutable" (in the sense that we also promise to not break implementors of the interface)?

In fact, if we want to support #39's idea of "public" interfaces that are not "published", then I think the appropriate PHPDoc tag for that would be @internal, right?

Looks good!

We should add @api to ContentEntityInterface and ConfigEntityInterface in this issue as well?

 /**
  * Provides an interface defining a config_test entity.
+ *
+ * Future major and minor releases might make API additions to this interface.
+ * In order to replace an entity class using hook_entity_type_alter() it is
+ * recommended that the new class should extend
+ * \Drupal\config_test\Entity\ConfigTest.
  */
 interface ConfigTestInterface extends ConfigEntityInterface {

Is it pointless to make any guarantees on a test entity type?

Am I understanding right that all Drupal interfaces are effectively "published" (in the sense that we will not break contrib/custom callers ever, except if absolutely necessary to fix a critical bug), but that ones marked @api are additionally "immutable" (in the sense that we also promise to not break implementors of the interface)?

I'd only add to that that nothing in an experimental module would be counted as 'published', so we might potentially break callers in those cases too.

I'm wondering if we really want the docs added in the patch, or should just add add a note to https://www.drupal.org/core/d8-bc-policy. If we do want to individually document interfaces patch looks fine though.

In fact, if we want to support #39's idea of "public" interfaces that are not "published", then I think the appropriate PHPDoc tag for that would be @internal, right?

This gets tricky, for example it's not how Symfony uses @internal - see http://symfony.com/doc/current/contributing/code/bc.html#using-our-inter...

@internal for them means "Don't use this at all". And even untagged interfaces they effectively do not allow any changes to whatsoever, there's no distinction with @api for those.

Also there are definitely cases in core where @internal for us means "don't use this at all, we're going to remove it", so feels like it muddies the waters for Drupal-only code too.

So I'd go for the following:

@api - immutable.

Not tagged - published - guaranteed for callers, not for implementors.

@internal or in an experimental module - do not use or be prepared to update if you do.

Question. Does this change the recommended practice for contrib modules that are creating entities? Vis, should they stop providing interfaces for their entities? Should they be documenting them differently?

(I am working on a module that has its own content entity right now, so this is directly relevant to me.)

The old approach would have meant that it would be recommended to no longer define an interface.

The current suggestion doesn't change anything. You could add the same explicit documentation, but my understanding of #47 is that not documenting anything is exactly the same as the changes in #42.

Here's a suggested wording for the bc policy page:

Interfaces

@api

Interfaces tagged @api can be safely used as type hints and implemented. No methods will be added, removed or changed in a breaking way.
Methods may be deprecated in minor releases.

Not tagged with either @api or @internal

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:

• Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
• Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

Interfaces tagged @internal or in experimental modules

In general you should neither type hint nor implement interfaces marked @internal or included as part of experimental module, since methods may be added, removed or changed, or the interface itself may be renamed or removed. If you have a good reason to use the interface, you should be prepared to update your code for changes. Where possible we will group any breaking changes into minor releases as opposed to patch releases, but this is not guaranteed.

The only change I'd make is to mention a trait along side the base class. We don't use that much, but many of our base class uses really should be traits instead (and thus have the exact same benefit in this case as using the base class). Otherwise, #51 looks OK by me.

I edited to add a note about 1-1 class/interface relationships, since that's what the original issue was about (i.e. neither a base class nor a trait but we're still encouraging people to inherit), and also for traits.

#51 looks good to me as well. The scope of this issue seems like reasonable to get into 8.1 now. Other than updating https://www.drupal.org/core/d8-bc-policy with something like #51, what do we want the scope of this issue to be?

1. Do we want to add @api tags to EntityInterface and friends in this issue?
2. Do we want to open other issues for non-entity-system-related interfaces we want to promise BC to implementors for?
3. Do we want to add docs to NodeInterface, etc. and/or to hook_entity_type_alter(), pointing to https://www.drupal.org/core/d8-bc-policy? Or is there no reason to call those out separately from other non- @api interfaces?
4. Do we want a change record once we update https://www.drupal.org/core/d8-bc-policy and/or commit some patch here?

+1 to #55, so I'm RTBCing #51.

@alexpott, @catch, and I all like #51, so removing the "Needs framework manager review" tag.

This issue was tagged as "Needs subsystem maintainer review" in #16, but that was when the direction of the issue was more invasive to the Entity subsystem. The new issue scope is much less invasive, and #36 indicates that @Berdir and @catch support it, so also removing that tag.

Also raising the priority to Major, because not doing this means either not adding any new functionality to any entity type, or doing so without giving fair warning to contrib developers, and both of those options suck.

And tagging for "beta target" because getting this into the release notes of the next tagged release means giving that fair warning to contrib developers at the time that they're getting their modules ready for 8.1, rather than after they've already done that.

I've updated the document. It doesn't perfectly fit with the current structure of the doc, but that's something we can refine as we go on.

Also opened #2689675: Tag EntityInterface and friends as @api.

I think we should get started with Entity interfaces before we open issues for other subsystems or a meta, because there's likely to be a fair few interfaces that need some discussion, and it'd be good to concentrate that discussion in one place first.

I agree we need a change notice, but didn't want to copy and paste text from the policy that could get out of date again if we change it, so I opened a catch-all change notice for updates to the policy since 8.0.0 was opened.

That should cover everything except the meta mentioned in #54 (if someone wants to open a meta go ahead, but I'd still want to postpone it on the entity issue if possible so we can get one thing done then apply it elsewhere).

So marking fixed. This was tricky to get right but feels like a great solution in the end.

Actually leaving this RTBC a bit longer and the change record as a draft in case there's comments, will come back an mark fixed a while later if no-one beats me to it, and we can publish the change record then.

I've updated the document. It doesn't perfectly fit with the current structure of the doc, but that's something we can refine as we go on.

I tried to make it fit a bit more: https://www.drupal.org/node/2562903/revisions/view/9493587/9496023.

leaving this RTBC a bit longer and the change record as a draft in case there's comments

The CR looks great to me.

OK that's three days. Went ahead and published the change record, and moving this to fixed. The meta is open in case something else comes up.