Updated: Comment #12

Problem/Motivation

There has been, in the past, no way to distinguish between functions, classes, methods, etc. that absolutely cannot change within a Drupal major version, and those that are allowed to change, except:
- Functions starting with _ can change
- Private class methods can change

It would be useful to have a concept of a stable, public, API for Drupal. We would allow things that are not part of this API to change at certain times, and require that things that are part of this API not change.

Proposed resolution

At least broadly, follow http://symfony.com/doc/current/contributing/code/bc.html

This means using the following two tags in core, for both classes/interfaces and individual methods:

@api means 'guaranteed backwards compatibility' - always safe to use the class/method or implement it.

@internal means 'likely to change' - don't use this, or be prepared for upheaval if you do.

Not tagged means we assess the impact vs. disruption of any backwards compatibility breaks, only make them when necessary, and ensure there are clear instructions to update. This means that code that is not tagged is in roughly the same category as 7.x or 8.x beta code now.

Remaining tasks

Agree on whether @api vs. @internal vs. untagged is a good distinction.
Decide what to do about ArrayPIs (mainly form and render structures, but really anything (route definitions etc.) that get passed to alter hooks.
Initially open issues to tag classes/methods as @internal - since this tag can't be applied to code once 8.0.0 is released.

User interface changes

None.

API changes

None. This is a policy, not an API change.

Original report by Crell

In order to make it easier to extend core without breaking APIs, we need to clearly define what our APIs are that we don't break. :-) In practice, there are different levels of "API-ness".

Our traditional approach here has been "a line that begins with 'function' can never change without it being an API break (unless the function begins with _, but then we still don't change it just in case)". That is an inadequate standard.

A common technique is to tag "for reals APIs that we're going to promise won't change" with an @api doc tag. We should do the same. That means deciding how we determine what should get tagged that way.

Standards we should NOT use:

* All public methods (this is needlessly redundant and over-promising)
* All protected methods, for child classes (this is needlessly redundant and over-promising)
* All methods that are part of an interface (again, needlessly redundant)

So, what methods, classes, and interfaces should we document as @api meaning "we promise this won't break unless we have to for security" as opposed to simply part of an interface which means "this is the interface and we're pretty sure it won't break, and we'll try really hard not to break it"?

Discuss.

Comments

klausi’s picture

I disagree on the interfaces part, all methods on all interfaces should be automatically considered part of the API.

Could give an example interface method in core that would be a candidate for not being part of our API?

jhodgdon’s picture

Title: Define rules for using @api » [policy] Define rules for using @api
Issue tags: +coding standards

Adding standard title/tag for such discussions.

I think this is impractical for the Drupal project at this point. There would be a lot of @api tags and endless bikeshedding to get to the point where @api was complete enough to be useful.

Also, you'd need to define here what could and couldn't change with things marked @api. Could they be marked "deprecated" later? How long would they need to last? Could you add new arguments to a function (presumably with defaults)? And if so, how would you indicate that some of the arguments are "part of @api" and some aren't?

catch’s picture

There would be a lot of @api tags

We don't necessarily need to do that up front. If we're cautious about what we tag, we can always mark more things over time. What we can't do is mark things as @api then break them anyway.

It's true that this means the @api tags wouldn't be particularly useful say only 50 methods across all of core are tagged, but it'll gradually get to the point where they are and that's a process that can happen with minor releases of core I think.

Could give an example interface method in core that would be a candidate for not being part of our API?

CacheInterfaceBackend::isEmpty() - it's only used in tests and it ought not to have been added in the first place, since many backends can't meaningfully implement it.

jhodgdon’s picture

RE isEmpty() ==> That sounds like a good reason to remove the method from the interface, not a reason to have an @api tag that tags all the other methods in the interface?

jhodgdon’s picture

I'm also wondering what, if anything, api.drupal.org should do with this @api tag if we adopt it.

It seems like maybe we should have a @defgroup api that describes what it means, and api.drupal.org could take all @api tags and treat them as though they were marked @ingroup API?

If we did that... we ran into issues with the @Annotation tag assuming it means @ingroup annotation, in that vendor files with @Annotation should not have been displayed there. Are Symfony and other projects using @api, and if so, are they using it in the same way? And would we want them included?

Crell’s picture

What is being proposed is basically modeled on how Symfony uses the @api tag. I've asked Lukas to stop by with more details of what it "means".

lsmith77’s picture

Here are some links:
http://fabien.potencier.org/article/47/pragmatism-over-theory-protected-... (talks about the need to have some methods be public despite them not really being part of the public API)
http://fabien.potencier.org/article/49/what-is-symfony2 (mentions that for the public API we maintain strict BC between minor versions)
http://symfony.com/doc/master/book/stable_api.html (the relevant documentation page)

jhodgdon’s picture

OK... So it sounds like this issue really has three parts:

a) Define the concept of "Public Drupal Core API": what does it mean, when and how is it allowed to change, etc. This seems like part of the much broader question that has been under discussion lately of how to manage change in Drupal Core in general, and it is probably too broad to discuss in the issue queue or on this issue? (We can also debate the name -- I just chose something here so I could refer to it.)

b) Once (a) is done, decide which hooks, functions, classes, interfaces, methods within interfaces/classes, and perhaps other things like Form/Render element definitions belong to the Public Drupal Core API. Probably this will involve many issues to agree upon each component that is added to the Public Drupal Core API, and again it is probably too broad to discuss in this issue?

c) Come to an agreement on how to keep track of which things are part of the Public Drupal Core API. The logical thing to do seems to be to use the @api tag, and have the API module make this into a list for api.drupal.org.

It seems to me that only (c) is really the subject of this issue?

If this seems like a reasonable summary... I'm in favor of adopting the seemingly standard @api tag (it is part of PHPDoc at least, and Symfony is using it). But I think we need to define (a) before we start deciding which things need this new tag. And if you want to expand this issue to include (a), then it needs a new title.

Mile23’s picture

An API is a set of instructions you follow in order to make Drupal do a thing for you.

Docblocks in methods are not the proper place to document such things.

Examples are. :-)

Creating a list of 'this will never change' methods means that you now have a bunch of classes/interfaces and methods that you can't ever change. This would be useful in a top-down design environment, but that isn't Drupal. The Stable API page linked above is excellent because the Symfony 2 folks had a design process where, as an early step, they determined that it was a good idea to refactor Symfony into independent useable chunks. Drupal has not gone through that process with nearly the same discipline, so it's difficult to determine where the API starts and ends. It's hard to compare the two.

By way of lighting a candle rather than cursing the darkness, I'd suggest a new rule: You can't tag a method with @api until that method is used in an example that has been folded into core. See: #1532612: Move Examples Project into Drupal core

Also: jhodgdon +a million. We're missing the first steps about what API actually means.

catch’s picture

Drupal has not gone through that process with nearly the same discipline, so it's difficult to determine where the API starts and ends.

Well that's the point of starting to use the tag, albeit cautiously at first. Also this will only apply to minor releases, we'll still be able to break bc completely for major ones if we need to (although likely API changes for things tagged @api will be looked at a bit more circumspectly). It makes more sense to differentiate if we adopt semantic versioning and the ramifications that has on the release cycle in general.

I'd expect us to start with low-level systems that have not recently changed much - for example the database and queue APIs could have 90%+ of their methods tagged as stable and we can be pretty sure there's no plans to change those any time soon. Something more recently added like plugins or the new entity/field API we likely can't tag much at the moment, but may be able to after a stable release or two.

Just getting into the habit of recognising what is and is not stable would be useful I think. For an example on #1693336: Block rehashing happens on every cron run chx wrote a bug fix there that ensures a side-effect of the bug itself still occurs (i.e. allowing modules to change block definitions arbitrarily and have cron pick that up). Side-effects of bugs should not be treated as part of the API but there's no way to differentiate at the moment.

Mile23’s picture

Well that's the point of starting to use the tag, albeit cautiously at first.

Well, that's the question, isn't it? :-) What's the consistent metric that will be applied to all of Drupal in order to make the determination?

I say if it's not used in a core example then it's not in the API, and if it *is* in a core example there should be an issue about promoting it to the coveted @api tag status. That's because having an API tag on documented methods is not the same as having API documentation.

Crell’s picture

Nor are examples synonymous with documentation. :-) I agree that anything tagged @api is a higher-priority to have examples using it, but I don't think it's a 1:1 mapping.

jhodgdon’s picture

Title: [policy] Define rules for using @api » [policy] Decide what the Drupal Public API should be and how to manage it

It seems like everyone wants to work on all of the parts of #8 here, so I am updating the title and I'll also update the issue summary.

You have just broadened the scope quite widely here to a very broad discussion.

Mile23’s picture

Nor are examples synonymous with documentation. :-) I agree that anything tagged @api is a higher-priority to have examples using it, but I don't think it's a 1:1 mapping.

Not a mapping, but a process. If it's not in an example, why would it be considered part of the API?

Mile23’s picture

Issue summary: View changes

Create an actual issue summary

larowlan’s picture

Priority: Normal » Major

Now we're nearing rc, this is major in my books (well I think its critical but others don't agree)

xjm’s picture

catch’s picture

To try to get this moving again:

There's very little risk in marking something @internal since that's the opposite of a bc-promise. The tag can just be removed again.

I think we could open issues for that - and it'll probably be quite straightforward to find things to mark with it.

@api is much harder to get right. There was another issue where we discussed bc-breaks in general which I can't find at the moment.

Also whatever happens we're going to end up with things that aren't tagged (i.e. no @api, @internal or @deprecated) and we should document what that means.

dawehner’s picture

The symfony project build up a pretty detailed documentation about their policies: http://symfony.com/doc/current/contributing/code/bc.html
There are good points in there, but I doubt we should promise protected $variables.

Crell’s picture

xjm and I started a document for this back in Austin after my core conversation there, but it fell by the wayside in the rush to Beta over the summer. We should probably resurrect that. I can't find the full doc we wrote right now, but an earlier draft of it is here:

https://docs.google.com/document/d/1f2GWZmlhAFS6ALt5-1QYE6RTmXG-KK_zdqXF...

And my core conversation (which got fairly good feedback at the time) is here:

https://austin2014.drupal.org/session/road-81.html

jhodgdon’s picture

I'm a bit confused about what the proposal is here?

rickvug’s picture

Something I've wondered is if all functions in core/includes/*.inc should all be marked as deprecated and open for removal in point releases, even if there isn't yet an OO equivalent. Wrappers could still be provided but it would nice to have a way to cleanly remove the functions entirely so that the legacy .inc files are not loaded during bootstrap.

larowlan’s picture

Here's what I'm after as a maintainer.

Let's take forum for example.

forum.module only contains hook implementations and theme preprocess hooks. I've worked hard to get it that way. It's been that way for at least 18 months.

Lets say 8.2.x comes along with an object oriented hook system.

I don't want to have to guarantee that all of the existing hook implementations will exist forever in 8.x, so before 8.0 comes out, all hook implementations in forum.module are marked @internal. This means, if you use them, do so at your own peril.

8.2.x comes along with its object oriented hook system and as forum maintainer I'm quick to jump on board. I move all of the logic code to object oriented implementations, mark all of the hook implementations as @deprecated to be removed in 8.4.x and use trigger error in the body to raise a deprecated warning, before calling to the new object-oriented code - so it continues to work.

8.4.x comes along, we can remove those functions. They were @internal to start with, and have been deprecated for 8.2.x and 8.3.x, with warnings triggered to notify.

If we didn't mark them as @internal before 8.0 came out, then in my opinion, you are safe to use them and if they were removed later, you'd have every right to be miffed.

On the converse, as forum maintainer, I'm committed to ForumManagerInterface and ForumIndexStorageInterface, I don't expect the public API to change, and if it does, then I'm committed to maintaining backwards compatibility. So to that effect I tag that entire interface as @api and are committed to keeping it around.

Those are the easy bits.

The hard bits are our array PIs. I have some ideas on that but will save them for later.

Does this sound reasonable? I have similar arguments for the other three modules I maintain, but not all of them are in as clean a state as forum.

benjy’s picture

I like the sound of #22, I think once D8 is released, the policy you mentioned for ForumManagerInterface should be applied across pretty much every interface in core?

catch’s picture

@larowlan yes that sounds pretty reasonable, one bit I'd be a bit looser on:

If we didn't mark them as @internal before 8.0 came out, then in my opinion, you are safe to use them and if they were removed later, you'd have every right to be miffed.

I think that applies to @api 100%. Not so sure for things that aren't marked either way.

In Drupal < 8 hook implementations aren't treated as part of a module's API and modules relying on them do so at their own peril, so while we might want to add an @internal to everything that already has @implements, I'm not sure you 'have every right to be miffed' if we don't - especially if we document at a high level that this is the case.

@bojanz

The hard bits are our array PIs. I have some ideas on that but will save them for later.

There was a good discussion about this in another issue, but I still can't find the issue despite much searching - we discussed what to do about changes to render/form structures etc.

jhodgdon’s picture

I am really unclear on what people are advocating here. A few clear alternatives would be helpful, preferably added to the issue summary, because I'm not sure whether people are saying "we need to mark everything @api that is part of the API and we can change everything else" or "we need to make a policy about what is implicitly part of the API" or what.

cilefen’s picture

+1 for @internal because my IDE respects it and gently warns you.

cilefen’s picture

Would it not be easier to assume everything is the API unless it is tagged @internal?

catch’s picture

Issue summary: View changes

@jhodgdon

So going by http://symfony.com/doc/current/contributing/code/bc.html the three states mean this:

  • @api - guaranteed no bc breaks under any circumstances
  • no tag - we'll try not to break bc unless we have to, and if we do then it should be a simple update - this is de facto what we do in 7.x and pretty much the same during beta phase
  • @internal - don't use this, or be prepared for upheaval if you do

In my view, we could ship 8.0.0 with absolutely zero classes tagged @api - we'd obviously assess impact of any possible changes, and lots of things in practice won't change, but we lose very little by adding the @api tag as time goes along. On the other hand if we tag things @api then end up changing them or having to do horrible things for backwards compatibility, then we've made things more complicated. This also means we can defer some of those discussion of what exactly gets tagged until RC or later.

On the other hand, anything that we want to tag @internal we should be doing that as soon as possible, since if we have a policy for 'untagged' code, then we can't suddenly change to a less restrictive policy for that code after release. In other words, any existing code added after 8.0.0 can never be marked @internal (only @deprecated).

David_Rothstein’s picture

So we currently have https://www.drupal.org/node/1527558 - it would be useful if proposals in this issue were explained in terms of suggested edits to be made to that page.

Adding @internal and @api is certainly new. Some of the other things (data structures, etc) already have a fair amount of detail in the chart on that page.

larowlan’s picture

@catch - #28 looks good to me - am I ok to open issues for forum/comment/block content/contact with initial stab at @internal tag? or wait until we finalise a policy here?

Aki Tendo’s picture

The assertValidCaller() method out of the fault patch is pretty much born to address this issue.

For those unfamiliar with that project, it adds assert statements to Drupal, which is code that is only checked in development. The assertValidCaller function is to be called from assert, and it makes the method it is in behave like a Java protected function, failing the assertion and erroring out if it's called from an illegal scope. The default scope is the callee's namespace but a scope can be passed as an argument to the function

So assertValidCaller('Drupal\Core') would only allow calls from somewhere in core to pass the assertion.

Unit tests can test to insure the assertion is thrown, then disregard it.

This works better than the @api and @internal labels alone because it's hard to overlook the asset fail error screen.

catch’s picture

@Aki Tendo - the thing with @internal is we don't necessarily want to prevent people from using it. iirc we're overriding/using some @internal Symfony methods ourselves. It just means that you have to be extra careful when new versions come out and expect to update things - and a warning is fine for that. To really prevent people from using things we could mark classes final and methods private in some cases - but we generally avoid doing both of those too.

One other thing to mention here. Part of the reason I don't think this should be critical, is because we can continue to tag things @internal or @api once we get to release candidate (and @api right up to 8.5.x). We just need to stop tagging things @internal after 8.0.0 itself is tagged.

Those are pure documentation fixes so don't risk introducing bugs during the RC-phase and it also means we'll be mostly out of surprise API-changes by then.

Aki Tendo’s picture

assertValidCaller() is a possible tool, no more or less. I'm not a fan of private and final either, but they do have their place.

Also if someone really wants to they can bypass the assertion by briefly turning assertions off before making the call that would throw the assertion. This is obviously not the intent of assert_option().

In my opinion class and method access should be kept as restricted as possible at all times. "But this might be useful" is a sham argument against the headaches of fixing compatibility breaks. The best way to fix those is not let them be created in the first place. I'll take "works but is inflexible" over "flexible but unreliable" any day.

Crell’s picture

It's more "it's not worth the effort to actively stop someone from calling it, just warn them so we have plausible deniability". :-)

My recommendation in Austin was that subsystem maintainers take point in determining what is @api and what is @internal within their systems/modules. I still think that's the best approach. My presentation had a number of recommended guidelines that we can publish as examples for maintainers to get inspiration from. Obviously any such patches would still need to get reviewed/RTBCed/committed so that provides a place for the framework managers to audit and ensure some level of consistency.

If we're good with that, we should do a writeup and post something to /core to notify subsystem maintainers that they should be thinking about this soon-ish.

Aki Tendo’s picture

It's more "it's not worth the effort to actively stop someone from calling it, just warn them so we have plausible deniability". :-)

By that logic, why use protected? Why use getters and setters at all - they just make the code harder to read. Do it the PHP 4 way, prefix it with an underscore, that's far better than this final, private and protected stuff - we don't need any of that right?

Not worth the effort? Puhlease. We're talking about 7 keystrokes to type the word final with a space here, or 8 for private. A stronger argument can be made against the assertion I wrote since it has to be unit tested and it is calling a function with a statement, but the work has been done so at this point it's one code line.

Which is harder - finding out you were overly restrictive on the scope of a method or class and need to relax it on a minor version release, or finding out that you can't change a class substantially without breaking who knows how many modules out there? Either way someone is handcuffed here - in the first case it's the third party developer, in the second it's the core dev; and if you go ahead and break everything anyway then everyone can be unhappy, especially end users who's sites stop working for reasons they don't understand. Who they going to blame and right the *this sucks* post about, the module or Drupal?

My money is on Drupal, so letting this sort of thing happen is at the very least bad PR.

I'll bow out of this now - I don't have any real standing around here so I know my opinion doesn't really matter to anyone - but for what it's worth this is it: If a problem can be avoided with reasonable planning and effort it should be avoided. Marking things programmatically takes more time and effort than PHP documenting things, but it's less likely to be misunderstood. And if you start failing unit tests by putting in an assertion that something shouldn't be calling something else then maybe, just maybe, the PHP doc note wasn't a good idea either. In the latter case you really can't know if you're correctly labeling something - you're stating an intent without enforcing or even testing it to see if it works.

Wim Leers’s picture

I don't have any real standing around here so I know my opinion doesn't really matter to anyone

You couldn't be more wrong! :) I think all of us truly appreciate your different POV on things. At least, I know I do!

I think what's key to understanding our hesitance around the use of final (and related matters) is simply this: it requires a level of certainty about the API that we rarely have in Drupal. Because we tend to design not for one use case, but for many all use cases. It's very easy to forget about some use case somewhere along the way.
At least, that's my understanding.

Like you, I'd like to see more strictness. I'd like us to be at the level of Qt in terms of API design, API docs, and how everything beautifully fits together, even if it means complex internals. We're getting there, but we're definitely not yet there.

David_Rothstein’s picture

I agree with Aki Tendo regarding private/final/etc; those are PHP's built-in tools for this, and we should be using them when possible. As he said, it doesn't require complete certainty about the API because you can always move something from private to public later if a use case comes along.

That doesn't mean there's no use case for something like @internal too (there definitely is), but I think the guidelines for this should recommend using private/final over @internal (in object-oriented code) except when there's a very specific reason not to.

jhodgdon’s picture

Hm... I'm not sure that the PHP modifiers cover all the cases. For instance, "final" only applies to classes and methods. It can't be put on functions, interfaces, class constants, stand-alone constants, or class member variables. So I don't think it quite corresponds with or replaces the proposed @api semantics. I'm not saying we shouldn't use "final" necessarily, just that @api in docs is more flexible than the PHP final keyword.

Similar for private -- it doesn't completely cover what is proposed for @internal.

Aki Tendo’s picture

Functions can be guarded by assert('Assertion::assertValidCaller()') - that's what I wrote that for. Constants are probably ok to be open but it wouldn't hurt to label them. Interfaces would need labeling though. There's little reason not to do both - I'm just concerned at avoiding the use of the PHP keywords.

Case in point - Drupal\Core\Template\TwigExtension. We aren't supposed to extend from that - well I'm here because I did, crashed my system, then spent three days debugging why. Putting an assert that the urlGenerator is set would have avoided that pain, but so would have making that class final since we do not want people extending from it - they should instead extend off \TwigExtension directly.

One cautionary point on final brought up on Sitepoint - final can interfere with Unit Tests by blocking mocks of that class. However the solution is to use an interface and mock off the interface - and Drupal already uses Interfaces instead of classes so this wouldn't be a problem.

David_Rothstein’s picture

I think final covers some cases that @internal and @api don't (what it enforces is different than what seems to be meant by either of those tags; it's sort of in between).

For private vs. @internal, they are much closer. The two cases that I think would be covered by my "except when there's a very specific reason not to" comment above (and where @internal would make sense) are:

  1. Procedural code (obviously).
  2. Code that you need to call from somewhere else yourself (e.g. a core method that another core subsystem needs to use) but that you really don't want anyone else to call.

Is there anything else? I would recommend documenting that @internal should only be used in those cases.

(And when @internal is used, using assert() to enforce that could be nice too... but I think it would be a separate issue because either way the @internal tag would be useful for documentation.)

Crell’s picture

Aki, please calm down. What you're missing is the 6 years of history around the question of public/protected/private that you weren't present for, which is a history that is very relevant and colors this discussion.

If you didn't watch the video I linked to from #19, please do so. There's a lot of context that I cover there, both in Drupal and other PHP projects.

The status quo in Drupal as of today is:

* properties are only ever protected, never private or public.
* methods are public or protected, but never private.
* Exceptions to the above are extremely rare.

The reason for that is we want to encourage people to use interfaces (methods) rather than directly accessing data structures; however, we also want to allow people to do things we didn't expect, because we're humble enough to know we're NOT going to think of every use case. We also know we've run into numerous cases extending Symfony components where we said "gah, if that property/method were protected instead of private this would be so easy but it's not so we have to do this garbage, GAH!"

final is generally eschewed for the same reason. (The only place I think we use it is the Database static class, which doesn't represent an object at all but a collection of functions for the database system. I dislike it, we tried removing it, but Simpletest got in the way. I hope to kill it eventually.)

As I note in the presentation, it really is a mistake to assume that public === API and protected === API for child classes. There are plenty of implementation details where you need to make something non-private without intending it to be part of the Public Supported API For All Users For All Time I Promise(tm). The point of using auxiliary documentation (@api, @internal, etc.) is to denote what is part of the For Reals API(tm), and what is just incidentally accessible. Case in point, ContentTypeHeaderMatcher is a class with public filter() and applies() methods, because the interface says they have to be (duh). But as the maintainer of that subsystem I do NOT view that class's existence as an API; the behavior it provides being something that someone can rely on, yes. That specific class doing it, no. I need some way to communicate that to module developers.

Put another way, there's 3 levels of statement to be made:

* We support you doing this.
* We do not support you doing this, if it breaks then it's your own fault.
* We will actively stop you from doing this.

Drupal generally avoids doing the 3rd, most of the time, so as to not box ourselves and module developers in too far. If someone really does have a reason to extend ContentTypeHeaderMatcher for some reason on their site, well, I'm not going to promise not to break it but I don't want to actively break their module if they try.

Being a hard-ass about not allowing something (eg, type hinting method parameters, assertions) is great when not following the type hint would make the code logically invalid, and I am a strong advocate for it. But when it's a case of "that likely won't work and could break in the future maybe" Drupal tries to avoid boxing developers in.

Hence, out-of-band data. Using @internal on Drupal\Core\Template\TwigExtension would still indicate to you "you really shouldn't be extending this, if you are then you're probably doing something wrong" without going as far as "BAD DEV, NO COOKIE, I don't care what your use case is, shove off!"

tl;dr: private/protected/public, @api/@internal, and final are not equivalent. They represent different things, and all have their appropriate use cases. Substituting one for another is a poor choice.

Fabianx’s picture

@Aki: Your opinion is very very much appreciated and I love the assertions work.

The TwigExtension is a good example to show Larry's points:

Lets assume for a moment I could not overwrite arbitrary twig functions by registering a new Extension with the same functions declared with lower (?) priority.

But instead lets assume I would need to exchange the service class to make changes (as the only option).

If the class was declared final my only choice to e.g. overwrite the drupalEscapeFilter to add some functionality, then call the parent, would be to copy and patch the whole class - like Drupal had to copy the Symfony YAML container parser, then patch it.

That is bad for maintainability and here Drupal is very different from Symfony.

However as a theme system maintainer I could totally say that TwigExtension is @internal, but still allow you to override it with your own class.

I also would say that drupalEscapeFilter is part of @api and won't go away during the stable release cycle and won't change function arguments.

With both annotations you got more clarity that what you want to do is maybe a little risky, but still possible within the realm of your own custom module.

That is the reason why I would not make the class 'final' even though for your own Twig Extensions you should not extend it, because even for my own sites I would see possible use-cases in exchanging the TwigDrupalExtension "service".

--

There is another reason:

Core has traditionally moved very slow and contributed modules move way way faster.

We will see if / how this changes after 8.0.0 with semantic versioning, but generally before the feeling as a contrib module author was:

"If core does not support it, either work around it or good luck (!) getting it eventually in (then wait a few years for your patch to be released) ..."

Case in point:

Forward revisions, part of Drupal 8 now for a few years, only possible in Drupal 7 with severe hacks or via very cleverly using the existing APIs (still a hack technically).

With Drupal 8 someone could have extended the Entity API storage service, extended it with the functionality they need and then possibly contribute it back. That would not be possible with totally strict APIs and final classes and private methods.

Yes, it seems "insane" from a PHP-library view point to allow something like that, but that is Drupal with its eco system of long-term core and short-term contrib and custom modules on your own sites.

Drupal has traditionally allowed via its hook system to change almost _everything_ and this is one thing we want to keep and extend.

Even in Drupal 7 you could overwrite e.g. the whole of taxonomy.module by using some special hooks ...

Drupal 7 made several things pluggable directly (drupal_http_request, path.inc, lock.inc) and is even now extending that to some point (variable.inc) ...

Yes, this is the reason why Drupal updates can be more difficult, but it is also the reason (IMHO) why Drupal is where it is today.

I would even say its the one reason I always liked about Linux and Drupal (and probably why I stuck with Drupal), because they are open systems without limiting me as a developer, but instead giving me unlimited flexibility.

In the Linux Kernel I can also change everything I want with loading a module, yes, even the most basic kernel code (compare ktrace / kdebug / dtrace / ftrace / etc.).

And yes I realize it is a different thought model from the rest of the PHP world, but that is fine.

This issue (and assertions can help there) is about documenting constraints / ideas for what we thought when we wrote the code. (oh this is a helper method and this is a public API method, this one could change one time though).

And that is why @api, @internal and to some extend assert() is useful outside of marking things private and final.

dawehner’s picture

I really strongly believe in freedom, not only in general, but also in software. If the software makes it hard to be reused, due to the overagressive use of private,
I think we are loosing part of that freedom.

PS: In an ideal world, PHP would have the friend keyword, so you would not have to make things public and mark it as @internal at the same time.

Here is my separation of the usecases:

@api Used for clearly defined interfaces OR clearly defined objects, like the URL object
None
@internal Used for objects which are used as part of the interaction process, but you should not use it directly in your code. Things like plugins / random implentors of interface shouldn't have to tag with @internal.

larowlan’s picture

Here's a concrete example for discussion #2494397: Document what constitues the api of forum.module

webchick’s picture

Reviewing the patch there, seems like we could eliminate a lot of typing by stating the following in API.txt or similar:

- Any given module's hook implementations are always @internal.
- So are sub-classes of core's base classes, e.g. BlockBase, ConfigFormBase (if those were marked @internal in 8.x core the @inheritdoc would pick them up, OTOH from the base class POV they are the @api. Tricksy. :))

Crell’s picture

webchick: Well, some of those base classes are intended for module developers to extend, thus should be @api. The two you cite are good examples of what I would classify as @api base classes. (BlockBase is so different from BlockInterface it's rather painful.) Those are also the few cases where I would support use of private in the base class, because extending them is the primary use case rather than the "escape hatch" it is in most other situations.

catch’s picture

@Crell that's not what webchick said:

So are <em>sub-classes</em> of core's base classes

(emphasis added).

So the suggestion would be "things that extend from base classes should be assumed to be marked @internal", not that base classes are @internal. I think this will be harder to document in a standard way than hooks, since while we have @implements and .api.php for hooks, base classes are less defined (is it anything with 'Base' in the name? is it any abstract class?)

Sometimes protected methods on base classes are intended to be used by implementing classes - i.e. they're part of the intended API for implementors of the class, if not for callers, so not sure how feasible marking any of those private would actually turn out to be.

plach’s picture

So are sub-classes of core's base classes

I'm not sure this is policy is generic enough to be applied: Views plugins are a typical example where extending a concrete class makes totally sense and can save you tons of LoC. I think at very least the main ones, for instance \Drupal\views\Plugin\views\field\Field, should be marked as @api.

Crell’s picture

catch: Hm, the sentence is unclear. I read it as "these are examples of @internal classes because they extend something else", rather than "these are examples of things whose subclasses are @internal". I think we're saying the same thing though: BlockBase and FormBase are @api, the various subclasses of those are @internal by default.

I think most of our "you're supposed to extend this" classes are named *Base. That's the rough convention we were working with the last time I looked, but I'm not sure how religious we were about it.

Sometimes protected methods on base classes are intended to be used by implementing classes - i.e. they're part of the intended API for implementors of the class, if not for callers, so not sure how feasible marking any of those private would actually turn out to be.

Sure, and those are the ones that should stay protected. However, if there are any bits of those base classes we do NOT intend for child classes to use, that's where private is useful. We'd have to examine each one to determine which is which. (Another job that IMO falls to subsystem maintainers to sort out.)

plach: Many of those should be traits then, rather than extending a concrete class directly. :-) Views still used a lot of D7-era class usage patterns (lack of interfaces, etc.) the last time I looked, so it may be more problematic than other parts of the code base. Again, this should fall to subsystem maintainers to sort out.

catch’s picture

So reading #48 I was thinking "OK so we shouldn't say sub-classes of base classes are @internal by default" - but thinking about it more, I don't think there's a problem.

If something is marked @internal, it doesn't mean we're going to change it, it just means we might and you should be prepared. So core Views plugins we can decide to not change them (based on impact vs. disruption given it's a common pattern to subclass them), regardless of how they're marked. Then later we can mark them as @api and that will be explicit. The very worst thing that can happen is someone decides to copy + modify instead of subclass because of the @internal policy.

And this further confirms to me that we should really concentrate on marking things @internal first, since that's the promise that it's fine to break later, and only start on @api after 8.0.1 or later.

plach’s picture

#50 makes sense to me.

Moreover, after reading #49, I came to the conclusion that, regardless of the use of traits, if a class has so much reusable logic it would probably make sense to provide an abstract parent (and mark it as @api), instead of forcing people to extend a concrete class.

plach’s picture

Anyway, I fully agree with Crell that we'll need subsystem maintainers to perform a case by case evaluation.

Crell’s picture

Catch: Are you suggesting then marking *nothing* as @api until post 8.0.0? I agree we should be very conservative with it for now, but surely there's something we can say "please use this" to. :-)

plach: We're drifting off topic there, so I will simply point you to my prior statement on the subject of abstract classes :-) : http://www.garfieldtech.com/blog/beyond-abstract

webchick’s picture

Yeah, I have to say that tagging nothing as @api doesn't seem like the right way to go. Why did we bother with beta blockers and the like, if not to firm up the major APIs so that module developers can depend on them?

plach’s picture

I interpreted #50 as a suggestion to be conservative when choosing between @api and @internal, since there's always time to release APIs to the public. If so, we are all on the same page, I'd say, sorry for the noise :)

catch’s picture

I'm saying we should concentrate all pre-8.0.0 effort on marking things @internal - since we'll only be able to do that for new code after 8.0.0, not any of our existing code until 9.0.0. This is extremely important to get right so we can refactor things later.

If there is time left for @api and there are things we can easily agree on to mark them before 8.0.0, then fine let's mark them, but if that happens in 8.0.1 then I would not care at all.

Why did we bother with beta blockers and the like, if not to firm up the major APIs so that module developers can depend on them?

If we neither mark something as API nor internal, then module developers 'can depend on them' - it just means there's a small chance we'll make small backwards compatibility changes in a minor release down the line.

This could be as small as adding a new optional parameter to a protected method - does not affect callers of the method at all, but would break a class that extends and implements its own version of the method. If you tag something like that @api, then you can't even add optional parameters because it would break that (in some cases theoretical or single-user) implementing class.

Crell’s picture

catch: Well, that depends on what we define @api to mean. We don't have to define it as "this class will never have a diff on a line with the word function". We just have to be clear on what we define it to be. Eg, I would say that in general a protected method is not @api unless the method itself is specifically marked, eg in a *Base class.

David_Rothstein’s picture

- Any given module's hook implementations are always @internal.
- So are sub-classes of core's base classes, e.g. BlockBase, ConfigFormBase

Treating all those implementations as @internal includes many functions that return form and render arrays (that's the case in #2494397: Document what constitues the api of forum.module too).

What does it mean to mark those as @internal, in light of the guidelines for acceptable form/render array changes at https://www.drupal.org/node/1527558 (which basically allow lower-impact changes but disallow higher-impact ones)? Would those guidelines be ignored in the case of an @internal function, and the function would be free to change its output however it wanted to?

If so, this would be problematic for code that relied on doing things like hook_form_alter(), no? The example in #2494397: Document what constitues the api of forum.module is basically marking a whole lot of the user interface of the Forum module as @internal, but surely there are modules out there that would want to alter it.

Crell’s picture

David: One of the major downsides of naked objects (what form/render arrays are) is that the data structure is the API. That is, yes, if we treat them as an API we basically can never touch them, ever.

The discussion around Austin last year and shortly thereafter suggested that we make *most* form definitions inherently @internal. If we don't, we can basically never do any UX improvements for fear of breaking someone's form_alter. And that undermines part of the goal of semver, which is allowing us to evolve the system in minor releases, including UX improvements. That said, there's selected forms we may want to be firmer about as we know a huge number of sites will rely on them.

"Some module might possibly want to rely on an internal data structure" is exactly what makes code evolution impossible. We've done a lot of work in D8 to hide those internal data structures in many places to resolve that problem, but on the other side we have to also say, process-wise, "this is an internal data structure; if you rely on it, you're doing it wrong."

A point of this thread is documenting where and how that's the case.

catch’s picture

With form alters I think we mostly need to document that alters aren't guaranteed to get an identical form structure all the time.

Since any other module on a site can also alter a form and change the structure, we should try to encourage module developers to assume that will happen in general. i.e. the very fact that you're in a form alter yourself means that any other module can also be altering the same form to be different from what was originally defined.

However if you take that information, code defensively and check for array key existence, then your form_alter will stop throwing errors if that array key disappears, but equally whatever it was you were trying to do, just won't run at all and will fail silently (unless there is test coverage). So there is not really anything useful to tell people except 'be careful and watch for changes'.

Note that this isn't just a problem with ArrayPIs - it's an issue with any alter pattern, for example route altering depends on the route name not changing (see example in https://www.drupal.org/node/2187643 - if the route name changes, then you can't find it and tough luck).

David_Rothstein’s picture

The discussion around Austin last year and shortly thereafter suggested that we make *most* form definitions inherently @internal. If we don't, we can basically never do any UX improvements for fear of breaking someone's form_alter.

That's not really true, since many kinds of UX improvements are already allowed in Drupal 7 (even if they change the form structure).

I guess it's confusing because https://www.drupal.org/node/1527558 says there's a "proposed framework" for evaluating such changes but in reality I think it was adopted in practice many years ago. As far as I've seen, we don't balk at making the lower-impact kinds of changes on that list at all. But the higher-impact changes basically never get made (critical issues only).

Hence my question, if we mark all (or almost all) forms and render arrays @internal what happens to that framework? - would a large change that completely reorganizes a form be accepted, even for non-critical issues? Those are the kinds of changes that could break people's form-alter code no matter what, whereas the lower-impact changes are the kind that are very unlikely to break anything (especially if people are coding their form-alters defensively as @catch describes).

webchick’s picture

Is there a @api-at-your-own-risk? :D

catch’s picture

Hence my question, if we mark all (or almost all) forms and render arrays @internal what happens to that framework? - would a large change that completely reorganizes a form be accepted, even for non-critical issues? Those are the kinds of changes that could break people's form-alter code no matter what, whereas the lower-impact changes are the kind that are very unlikely to break anything (especially if people are coding their form-alters defensively as @catch describes).

So this is tricky but I think it comes down to the following:

1. We can document that all forms are @internal by default
2. We can make not-likely-to-break-much changes freely due to #1
3. If we think a change really will break things, we can try to provide a bc layer like for example shipping 8.3.x with two different versions of the node form available.

Just because we mark the node form @internal doesn't mean we wouldn't think carefully about the likely real-world impact of a change. Also a major re-working of that form doesn't just impact modules, it might impact sites that have lots of documentation written with screenshots that would need updating, so providing a bc implementation of the form would allow individual sites to opt-out too. Or significant markup changes to the node (or comment, or user login) forms are quite likely to break themes since those forms are end-user facing on many sites.

On the other hand a complete re-organisation of admin/config/development/logging is extremely unlikely to break anything, and definitely isn't going to render someone's content editing documentation outdated. But for the purposes of documentation for developers, I think we can mark both as @internal (or rather default to that) to start with.

Crell’s picture

Concur with catch in #63. Reiterating: "We reserve the right to change this thing" does not equate to "we're going to unthinkingly change this at random". Just "caveat coder, we're not promising you anything on this one". (Which is part of why I think we *should* promise things on at least some parts of the system, so developers have at least something they can rely on.)

jhodgdon’s picture

OK... but ... hm...

So, if there's some critical/major bug that comes up, we might choose to modify even something marked @api in order to fix it. And we might never modify some particular thing that's marked @internal. Right?

We're already pretty careful about changes to the API after release, and we've always considered the impact when we make API changes... I am becoming less and less convinced, the more this discussion goes on, that @api and @internal have any meaning at all beyond what our policies have always been: that we'd consider the impact and benefits before making any API change after release (or after RC or Beta or whatever stage).

It seems like a lot of trouble to go through all of (or a lot of) our classes and methods and try to decide ahead of time whether it's a "low-impact" or "high-impact" class/method, when we don't have any benefits to weigh the impact against. Isn't that kind of a waste of time? It also seems like this doesn't change our impact/benefits calculation much to do it ahead of time?

Crell’s picture

jhodgdon: No, something marked @api we're saying we explicitly won't break, unless it's necessary for a security fix (which is already an exception we make). If something is unmarked, we try hard not to change it but we don't guarantee it. If it's @internal, we don't even guarantee that the class/method continues to exist.

See the Symfony BC Pledge for background: http://symfony.com/doc/current/contributing/code/bc.html

While I don't think we should adopt that exact definition verbatim, that's the general framework I believe we should be aiming for. Keeping the @api list small and contained for now, as catch suggests, helps to give us wiggle room in the future but still provides some level of safety for module and site developers.

David_Rothstein’s picture

OK, the above several comments make sense but in that case, for most forms and render arrays I agree with @jhodgdon that they should not be labeled "@internal". I think it's pretty confusing because @internal implies "you shouldn't be accessing this in your own code" but we don't want to imply to people that they shouldn't be writing hook_form_alter() implementations. It definitely confused me, at any rate.

So rather than what #63 says - "We can document that all forms are @internal by default" (which would mean the individual forms don't get the @internal tag added to them anyway) - I would suggest just documenting what the actual rules for forms and render arrays are. Kind of like we already do, but maybe make it more explicit.

Those rules would apply for any form or render array that isn't marked otherwise. If someone marks a particular form or render array as @api that would indicate that they really do never plan to change it except to fix a security issue, and if someone marks one as @internal that would indicate they are willing to revamp it completely someday for any reason, without any regard to code that might be altering it. But those would be the exceptions to the rule.

Crell’s picture

#67 makes sense. So we document "an untagged interface means X, an untagged class means Y, and if it's an untagged Form class specifically it means Z".

I'm good with that approach.

andypost’s picture

About "wrappers" like #2492585: Deprecate comment_view() & comment_view_multiple()
I think better to mark them deprecated for 8.x to maintain less API
Maybe better to file separate issue for them?

catch’s picture

Opened #2550249: [policy, then meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation to try to get something actionable happening for @internal, since I think that has to happen before 8.0.0.

We can talk about @api here still.

Crell’s picture

webchick’s picture

Formally escalated this to a RC1 checklist item, so removing the tag.

xjm’s picture

alexpott’s picture

Over in #2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release we've been trying to resolve the wish to make API additions to entity interfaces. We considering making them @internal or @deprecated however never solution turned out to satisfy everyone for good reasons. #2661926-36: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release (comment 36) @bojanz summed up a long discussion on IRC about the issue - that we should

Document that we reserve the right to expand non-public interfaces in minor releases.

The suggestion is that we document this by saying that interfaces that are not tagged with @api can receive API additions in minor releases. Interfaces tagged with @api will not change in minor releases.

jhodgdon’s picture

Um... But the whole notion of "non-public interfaces" is a bit crazy, isn't it? Why would we want to go to the trouble of defining an interface if it is "non-public"? I thought the point of interfaces was to define the public behavior of classes. We have, IMO, gone too far on the idea of interfaces if some of the ones we have in Core are "non-public". So maybe the right answer would be to get rid of any interfaces that have only one implementing class and that we would consider to be "non-public"?

On the other hand, there are some missing interfaces in Core, or at least there were last time I checked. For instance, it seems to me that every service should have an interface, because if a contrib module wants to override that service, they need to know what methods on it could be called. Last I checked, there were at least some services without interfaces.

alexpott’s picture

@jhodgdon - hmmm yes "non-public" interface is wrong. What we're actually trying to tease out is something along the lines of this http://martinfowler.com/ieeeSoftware/published.pdf.
@effulgentsia put it well in #2661926-40: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release

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)?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.