Problem/Motivation

See #2116841: [policy] Decide what the Drupal Public API should be and how to manage it.

Proposed resolution

Get the draft policy below finalised and added to either the handbook or api.drupal.org or both.

For now, documentation on d.o lives at https://www.drupal.org/node/2562903/

Original draft by xjm and Crell at DrupalCon Austin.

Open sub-issues for things not covered.

Drupal 7 backport policy: https://drupal.org/node/1527558

This document defines what parts of the Drupal codebase are considered stable and reliable APIs, and which parts module and theme developers should not rely on. The goal is to provide a greater degree of clarity about what constitutes Drupal's APIs and what constitutes implementation details.

Minor releases vs. patch releases

Drupal core patch releases (8.y.z) will contain security fixes and bug fixes only. No new functionality will be introduced, and no refactoring will be included except that necessary as part of a security or bug fix. See the Drupal 7 backport policy for more information.

Minor releases (8.x.0) may include non-API-breaking refactoring, new features, or enhancements of existing features. In such cases the core team will work to ensure that these enhancements do not alter the existing public-facing API of core systems.

Necessary security hardening takes priority over API stability.

We will make every effort to address security issues without affecting the public API. However, in some cases it will not be possible to address a security vulnerability without an API change. In such cases we will work to minimize the scope of the API change and document it thoroughly.

Backward compatibility (BC)

When we do make changes in either minor or patch releases, we will make reasonable efforts to provide a backward compatibility layer (BC).

We will take reasonable efforts to not break code that developers may be relying on

While core developers may change implementation detail code between minor versions when it is necessary to make an improvement, we will make a reasonable effort to minimize these changes.

The following parts of the code base should always be treated as internal and are not guaranteed to not change between minor versions:

The database schema for core modules
While the database query API and schema API itself are stable, the schema of any particular table is not. Writing queries directly against core tables is not recommended and may result in code that breaks between minor versions.
Render arrays (including form arrays)
While the render and form APIs themselves are stable, the exposed data structures used to build and cache pages and forms are not. We will change these structures where necessary to make usability improvements or add features in minor versions.
Markup, templates, and CSS
We will change the markup and templates for specific user interfaces for usability or feature enhancements, but we will avoid broad changes to markup or CSS that will affect more than one user interface. The Bartik and Seven themes are considered internal to core, and contributed themes should not rely on BC for those themes.
Tests
The contents of automated tests provided by core modules are never considered part of the API. While the testing framework itself may be treated as an API individual test classes are always internal.
Controllers
Core modules contain many route controllers that are bound to individual routes. These controllers are not part of the API of the module unless specifically marked with an @api tag on the method or class.
Strings
User-facing strings may change between minor releases. However, any significant change to strings must be committed at least two months prior to an expected minor release to allow localization teams time to update translation sets.
Constructors for service objects, plugins, controller
The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the fist place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

catch created an issue. See original summary.

catch’s picture

Issue summary: View changes
catch’s picture

Category: Task » Plan
Crell’s picture

Issue summary: View changes

I'm unsurprisingly on board. :-) Minor language tweak while I'm here...

For constructors, I would say those are assumed @internal unless otherwise noted. Otherwise we severely limit our ability to refactor internally if we want to add/remove a dependency to a service (which shouldn't matter to any users of the service) or split code out of a service into a new dependency (which I know we'll want to do in several places).

Saying that may also help nudge people toward composition instead of extending a class, which is a generally good guideline.

catch’s picture

Constructors are in the issue summary, how is #4 different? Or just the 'unless otherwise noted'?

Crell’s picture

Issue summary: View changes

Ah, the constructor subsection had no text so that read like it was still TBD. Updating with a description to make it clearer, and incorporating a brief conversation with xjm at MWDS yesterday.

Gábor Hojtsy’s picture

User-facing strings may change between minor releases. However, such changes must be committed at least two months prior to an expected minor release to allow localization teams time to update translation sets.

Discussed with @catch:

[5:55pm] GaborHojtsy: catch: it depends on the extent of change I guess
[5:55pm] GaborHojtsy: catch: and how much we want the translations to be updated
[5:56pm] catch: GaborHojtsy: also is that changes vs. new strings?
[5:56pm] GaborHojtsy: catch: if we are to add 3 new modules with hundreds of strings in a release and want translations to be updated :D
[5:56pm] GaborHojtsy: catch: well, currently l.d.o considers any string change as a new string
[5:56pm] catch: GaborHojtsy: I can see not adding new modules 2 months before a release date anyway.
[5:56pm] catch: But changes to existing strings, not so much.
[5:57pm] GaborHojtsy: catch: right, well, then making a sweeping string cleanup, like introducing string changes similar to https://www.drupal.org/node/2532634
[5:57pm] Druplicon: https://www.drupal.org/node/2532634 => Add Simple English #2532634: Add Simple English => 11 comments, 1 IRC mention
[5:57pm] catch: GaborHojtsy: yeah that'd still be loads.
[5:58pm] GaborHojtsy: catch: string changes would ideally come with a translation memory recommendation for translators and/or fuzzy matching, but we figured out neither
[5:58pm] GaborHojtsy: catch: so they are simply new strings atm
[5:58pm] GaborHojtsy: catch: I think it all comes down to the extent vs. urgency
[5:58pm] GaborHojtsy: catch: it would be pointless to not add 3 new strings / change 3 strings for a security fix because strings are frozen
[5:59pm] GaborHojtsy: catch: translators can always pick up the missing pieces AFTER the release, and D8 will autoupdate the translations later (unless the feature enabled by default is turned off for obvious auditing reasons)
[5:59pm] GaborHojtsy: catch: so its mostly a matter of how complete translations we want at the time of release and hwo much chance are we giving to people
[6:00pm] catch: GaborHojtsy: hmm OK, so that point needs some discussion regarding when minor release freezes are.
[6:00pm] catch: GaborHojtsy: will comment on issue.
[6:00pm] GaborHojtsy: catch: ok
Crell’s picture

Re #7: Do we have a sense of how much effort/time is needed per string that needs to be updated? Eg, if updating a dozen strings is no big deal, but updating 2000 is a crapton of work, where's the threshold where it becomes "a crapton of work"? I've no idea myself; The point of the amount of work needed for translations is valid, but we need some metrics/guidelines about how much work we'd be creating for people and how much lead time they'd need.

Gábor Hojtsy’s picture

@Crell: someone with some number crunching interest and adequate time could figure that out yes. I have the interest but not the time in the foreseeable future unfortunately. Timestamps of the following are logged and can be analyzed:

- When was a release tgz created on drupal.org
- When did localize.drupal.org download/extract/parse it
- When were suggestions for the given strings for that release entered (and by whom) on each translation team one by one
- When were said suggestions approved as translations by the reviewers (in some teams will be the same timestamps as the prior ones if they skip reviews)
- When did l.d.o export the .po files for that project with the updated translations

There is nearly no translation team who translated even Drupal core 100% (let alone contribs) so we would need to set some threshold for considering a translation "complete", but we could tell how many complete languages there are and then how much time (and how many people) did they involve in making that happen from release to release. (We could also look at the relative number of new strings in a release vs. time spent on it). That is the kind of data that is directly modeling the question you are asking we predict for future core releases.

Without the adequate time to spend on at least looking at the past, I cannot responsibly make predictions unfortunately.

jhodgdon’s picture

Issue tags: +Project governance

The issue title here has "@internal" in it, but there is no mention in the current summary about a tag that indicates other things that should be considered part of the internal API. Is this planned? If so, I think we need (a) another item in the issue summary section that about what should be considered internal, and (b) eventually we'll need some documentation on node/1354 as well as a patch to the API module to cover this tag.

Also... if we define this tag, are we planning to put @internal on all of the "automatically considered internal" things listed in the issue summary?

Also adding "Project governance" tag, as this seems like Drupal Core project policy?

jhodgdon’s picture

webchick’s picture

Unlike @api, my sense is we do need to figure this out before RC1, and @jhodgdon agrees. Adding the tag.

webchick’s picture

Title: Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation » [policy, then meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation
Issue tags: -revisit before release candidate

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

Also re-purposing to a policy issue, since the policy is what needs to be figured out prior to RC, the actual documenting can be done up until 8.0.0.

webchick’s picture

Issue tags: +8.0.0 deadline
Crell’s picture

So other than deciding if 2 months is enough leeway for translators, is there anything else we need to do here before it is handbook-able? And what would the next steps be? Eg, when/if subsystem maintainer should mark anything @internal...

catch’s picture

Issue summary: View changes
catch’s picture

Status: Active » Reviewed & tested by the community

Slightly clarified that 2 months for strings should be 'big changes' - if there's a spelling error we should fix that whenever (for example).

The render array item could use a bit more details - i.e. while we might change forms and controllers quite freely, I doubt we'll make big changes to render elements. But we can always tighten that up later.

Can't think of anything else. If we add this to the handbook, we can always add more things that are blanked @internal up until RC, just not after.

Moving to RTBC.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Don't we still need to add an item to the DL list in the "BC compatibility" section that says something like:

Things marked with @internal

Any class, interface, function, method, etc. that has "@internal" in its documentation block is explicitly designated as not part of the API, and can be changed between minor versions in non-backwards-compatible ways.

or something like that? There is currently nothing about that there.

David_Rothstein’s picture

Status: Needs review » Needs work

The issue title here has "@internal" in it, but there is no mention in the current summary about a tag that indicates other things that should be considered part of the internal API.

I agree, I don't understand the relationship here between the issue title and the issue summary.

The issue summary proposes an entire new policy, but there's no need for that. We should just be making changes/edits/additions to the existing policy at https://www.drupal.org/node/1527558 instead. For example, regarding translations, the existing policy has a lot more nuance than what is being discussed in this issue.

And regarding render arrays, the issue summary here says this:

While the render and form APIs themselves are stable, the exposed data structures used to build and cache pages and forms are not. We will change these structures where necessary to make usability improvements or add features in minor versions.

But that's very different from what we discussed at #2116841: [policy] Decide what the Drupal Public API should be and how to manage it and which everyone agreed to there -- or at least, what me, jhodgdon, and Crell agreed to there, and what no one has chimed in to disagree with the past 3 months :)

---

I think for @internal specifically, we should just add a section at the top of https://www.drupal.org/node/1527558 explaining what @internal means (and @api too) and then point out that for things that aren't tagged as @internal or @api, consult the table below for what general rules apply.

David_Rothstein’s picture

I just went in now and retitled https://www.drupal.org/node/1527558 from "What patches may be backported to a stable release of Drupal core?" to "What patches may be added to a stable release of Drupal core?".

That's a better title no matter what (even some Drupal 7 patches aren't relevant to Drupal 8 so were never "backported"), but especially relevant if Drupal 9 won't be opened for development until a while after Drupal 8's release. (Hat tip to @davidhernandez for pointing out this confusion to me a while ago.)

catch’s picture

The retitle makes this the obvious place to put the documentation. Although I think we should still somehow differentiate between 7.x and 8.x (to the point of separate pages if we need to).

#19 so I think the gist of the discussion in the other issue is the same as here in terms of what changes might happen, the difference seems to be on whether to use @internal or not.

However, I'm not sure #2116841-67: [policy] Decide what the Drupal Public API should be and how to manage it is necessarily a good reason to avoid @internal.

For example, I occasionally see (and have also written) 7.x code that looks similar to this:

function my_module_form() {
 $form = some_other_module_form();
 $form['foo']['#markup'] = 't(This is miiine, alll miiine now');
 return $form;

@internal indicates that this is explicitly unsupported - you should never directly use a render array and assume it'll not change.

hook_form_alter() you get given a render array which may already have been altered significantly, so you already have to assume it might change. And you're not using the render array as such, you're altering it where it's already used.

The useful distinction between @internal vs. untagged vs. @api for me is:

@internal means - we'll change this if there's any good reason to and if your code breaks it's your fault.
untagged means - we'll try not to change this unless there's a good reason to, and when we do we'll try not to break your code too much.
@api means - we won't change this unless we have to.

Even a small change to a form could completely remove an array key or value that an alter might rely on, resulting in notices or fatal errors worst case, so @internal seems consistent with that.

Symfony just does not have anything equivalent to a form array or alters - especially not something defined by the framework itself, so their "don't use this ever" while it might apply to the classes producing those things, just cannot translate to the world of alters. I don't think that's a reason to avoid using @internal for things, but it's a reason to not just rely on their policy since we have concepts that it can't cover.

David_Rothstein’s picture

@internal means - we'll change this if there's any good reason to and if your code breaks it's your fault.
untagged means - we'll try not to change this unless there's a good reason to, and when we do we'll try not to break your code too much.

I agree with that.

Even a small change to a form could completely remove an array key or value that an alter might rely on, resulting in notices or fatal errors worst case, so @internal seems consistent with that.

But there are different kinds of changes, and some are much more likely to break things than others. See https://groups.drupal.org/node/210973#comment-701613 which is currently linked to from https://www.drupal.org/node/1527558 (it's linked to as a "proposed framework" but in reality it has become de facto policy).

Based on that, to me @internal would mean we can change anything at will. Untagged would mean we're willing to make the lower-impact changes from the above link, but not the higher-impact ones unless there's a really good reason (such as fixing a critical bug).

catch’s picture

Right removing something from a form is different to adding #prefix or similar, and that's a useful framework for evaluating the impact of a change.

However while removing something from a form might be high-impact if something is relying on it, in practice there are dozens of forms in core (like nearly every admin config form) where it's very unlikely to cause a problem, or even if it did might result in one or two modules having a 5 minute upgrade path for a minor release.

For that I think the general uncertainty of @internal applied to all forms (treat what you get in form_alter as extremely unreliable) is preferable to indicating we won't make changes like that unless something is tagged we'll avoid a BC break with the array structure.

In practice, if it's the node submit buttons or the search form, we're not going to make a backwards incompatible change without a very good reason - but this is the exception. Also even in those cases we might provide an alternative form enabled by default for new installs, and if modules wanted to make similar changes to those forms, they'd need to add a new alter for the new forms to work as they previously did (on the installs using the new forms).

webchick’s picture

Assigned: Unassigned » webchick

Futzering with this in the handbook atm.

webchick’s picture

after some back-and-forth with @catch and @David_Rothstein on IRC, here's a swag at what the updated table might look like:

https://docs.google.com/document/d/13OpWueafN7iELme4JJNzsj4hrzdSHX_yOH2H...

webchick’s picture

Ok, various folks (@catch, @xjm, @Crell, etc.) have stated that it's better for the D8 guidelines to be separate from the D7 guidelines, given D8 has different rules / code constructs than D7, so I'm going to scrap the idea of combining both on https://drupal.org/node/1527558. It might be possible to do something like this later, once this new page is fleshed out enough to find that 90% of the page is the same, but that doesn't need to block RC1. Sorry, David.

Said page is under the "Backport policy" section https://www.drupal.org/core/backport-policy which doesn't seem quite right for this. Not sure what to call it. Symfony calls theirs "Our Backward Compatibility Promise": http://symfony.com/doc/current/contributing/code/bc.html Our statement about BC is at https://www.drupal.org/node/65922 which is under the "About Drupal" section https://www.drupal.org/about.

So... basically, no idea where to put this page or what to call it. :P I'm going to take a stab and throw it under https://www.drupal.org/developing/api/8 and we can always re-parent it later.

webchick’s picture

Component: base system » documentation
Status: Needs work » Needs review

K, let's try this: https://www.drupal.org/node/2562903 Note that I inverted the order a little bit and put a preamble before the definition list to explain what the different classifications mean (not married to any of that, was trying to imagine what info someone coming here would be looking for first).

Also, moving to the documentation component for better visibility.

jhodgdon’s picture

I took a quick look at this and here are a few thoughts:

a) Title... not a big fan... Maybe something like
- What Constitutes the Drupal API?
- Stable API vs. Things That Might Change [not literally this but something that gets this across?]
Not sure, it's late and I'm not creative.

b) First paragraph, great! Would like the title to get across what this says in 8 words or less. ;)

OK after reading this I propose for the title:
- Drupal API vs. Implementation Details

c) API definition section...

Hm... For this document I think you need to standardize on that if you say "Drupal API" you really mean "those things that are stable", not "anything that is part of Drupal Core". Because in the first paragraph of this section it seems like "Drupal API" is meaning "anything". Such as:
... etc. in Drupal 8's API have...
So I think here you should say "Drupal core's codebase".

d) Also I think we need to be clear in this document whether you're talking just about Drupal Core or whether this should/could/might also apply to Contrib projects.

e) I think in the API definitions sections and headers, you should *not* put @ before API and Internal, because that kind of implies that those tags will be visible.

f) Then in the specific sections, you should have an entry saying for instance:

- Anything with @api in its documentation header is also considered to be part of the API. [in the api section]
- Anything with @internal in its documentation header is also considered to be internal [in the internal section]
- Anything not otherwise determined to be @api or @internal is considered neutral [in the neutral section, which doesn't exist but maybe should?]

Because the rest is defining things that are *automatically* @api or @internal, right?

g) Also to be clear, if a class has @api or @internal in its header, does that mean that methods also inherit this? Or that it's a default for the methods? Not to mention the member variables if they don't have something contradictory? That needs to be clarified.

OK... sorry I am tired. This is mostly very good. Take anything I said here with a dose of salt and spelling corrections etc. because I need some sleep...

webchick’s picture

Assigned: webchick » Unassigned

Thanks a lot, Jennifer! Unassigning since I am not exactly sure when I can get back to this (anyone else feel free to take over; I'm just trying to get the RC1 checklist moving :)).

David_Rothstein’s picture

Status: Needs review » Needs work

Ok, various folks (@catch, @xjm, @Crell, etc.) have stated that it's better for the D8 guidelines to be separate from the D7 guidelines, given D8 has different rules / code constructs than D7, so I'm going to scrap the idea of combining both on https://drupal.org/node/1527558.

I don't understand what that means.

I see two instances in https://www.drupal.org/node/2562903 that are D8-specific, and both are just because of jargon:

  1. Controllers

    Can just be "Controllers and menu callbacks".

  2. Constructors for service objects, plugins, controller

    This is a mouthful, but it's just listing examples. The actual meaning is in the paragraph underneath it: "These are objects where developers are not expected to call the constructor directly in the fi[r]st place", which is certainly not a D8-specific concept (see, for example, the D7 cache API).

We should not have two separate policies for this, which contradict each other, and with the new one missing key information from the old one.

This issue is becoming more complicated than it needs to be. We just need to document what @internal is and how it applies to various things in Drupal and add it to the existing documentation, that's it.

https://docs.google.com/document/d/13OpWueafN7iELme4JJNzsj4hrzdSHX_yOH2H... is a good start at editing the current page and is much easier to understand. But we don't actually need 5 columns as shown in that document, only 4. "Notes for developers" is superfluous since anything in it could either be in the "Examples" column (possibly renamed back to "Description" like on the current page), or in the "D7" or "D8" columns if it's version-specific. We do need the separate D7 and D8 columns, although I would not expect too many differences between them; any difference is essentially a policy change being proposed in this issue (unless it was decided somewhere else previously that I'm missing?) so needs some discussion.

webchick’s picture

So the concerns voiced to me were:

- if I'm developing for D7, I don't want a bunch of confusion about D8, and vice versa. new 8.x developers might never touch a D7 site.
- things that are VERY important in D8 like class inheritance do not matter at all for D7
- the policies between D7 and D8 regarding what we allow in are already different, and likely to diverge further as time goes on
- while some concepts overlap, others don't (services, config entities) which results in a bunch of "N/A" for D7
- some terms are quite different between D7/D8: for example, "plugins" mean something completely different in D8 (Plugin API) vs. D7 (CTools Plugins) which could create confusion
- we do not want to block making release management decisions on D8 on "what about D7?" which we would probably need to do if the document covered both
- a lot of places where things aren't explicitly specified in D8 like they are in D7 are being kept deliberately vague to give us some more wiggle room (e.g. not treating render arrays as APIs like they are in D7)

Hopefully the folks with these concerns can just come here and talk about them directly. :) For my part, I am neutral, I just want to destroy blockers to RC1 with fire. :D

However, those concerns seem significant enough to keep the separate page. And according to xjm/catch that is enough to move this issue from RC1 checklist to release checklist. We'll likely refine this more in Barcelona.

David_Rothstein’s picture

Thanks for the summary. I think most of those are small things that a D7 vs D8 column would deal with, and some aren't really true.

But my bigger question is about this:

the policies between D7 and D8 regarding what we allow in are already different,

In what way are they already different? I thought the point of #2116841: [policy] Decide what the Drupal Public API should be and how to manage it and this issue is to decide on changes to the policy, and then document them.

I'd like to suggest that regardless of whether we wind up with separate D7/D8 pages in the end, can we at least formulate this discussion in terms of what big-picture changes we would make to the current page to turn it into a D8-ready page? Starting a whole new document from scratch makes it impossible to review what changes are intentional policy changes and what just got left out by accident. For example, on the topic of string changes which was discussed above, the current policy says this:

String change (admin-facing)
Description: Any changes or additions to any admin-facing string (string wrapped in t() or st()). Forces translators to translate new/changed strings, and multilingual sites see English strings until this is done.
Guidelines: Release notes mention, but string-affecting patches will be committed as early in the monthly release cycle as possible to give translators time to prepare.

String change (user-facing)
Description: Any changes or additions to any user-facing string (string wrapped in t() or st()); for example comments or the log in form. Forces translators to translate new/changed strings, and multilingual sites see English strings until this is done.
Guidelines: Major/Critical string problem only. (e.g. typo, string is saying completely wrong information)

String change (error message)
Description: Any changes or additions to any error messages (string wrapped in t() or st()); for example form validation errors or error messages coming from exceptions. Forces translators to translate new/changed strings, and multilingual sites see English strings until this is done.
Guidelines: Major/Critical string problem only. (e.g. typo, string is saying completely wrong information); imagine trying to debug a problem on your site in Martian. :)

But the new page just says this:

Strings
User-facing strings may change between minor releases. However, any significant change to strings must be committed at least two months prior to an expected minor release to allow localization teams time to update translation sets.

So in reviewing this I don't know if the intention is to change the policy to remove any distinction between admin-facing strings and user-facing strings (which I think would be a mistake - especially because it's not just about translation but also per-site string overrides), or if the distinction was just not considered when that paragraph was being written.

And similarly for several of the other categories.

David_Rothstein’s picture

I added a note at the top of https://www.drupal.org/node/2562903 now pointing out that it's still under discussion, and with a pointer back to this issue.

And according to xjm/catch that is enough to move this issue from RC1 checklist to release checklist.

I agree, this issue is not really related to the RC, but it is important for the Drupal 8 release.

catch’s picture

So in reviewing this I don't know if the intention is to change the policy to remove any distinction between admin-facing strings and user-facing strings (which I think would be a mistake - especially because it's not just about translation but also per-site string overrides), or if the distinction was just not considered when that paragraph was being written.

So the policy here is really about what changes can go into a minor release of Drupal 8, since none of these should even be considered for a patch release unless there's an exceptional critical bug that needs an API change and it really can't wait.

.For minor releases afaik the plan is that we'll make significant string changes if there's a good reason (for example an entirely new standard for hook_help(), or a new help API added which we convert core modules to use). In that case there would be significant disruption to translations, however:

1. There'd still be a two month string freeze before the minor release
2. In reality we're not going to do this every six months, but there could be a couple of big updates (!placeholder, help) during the 8.x cycle.
3. I don't think anyone has any intention of a 4+ year 9.x release cycle, if we're iteratively improving strings during 8.x then the 9.x translation update is likely to be more like a minor 8.x release (at least for translators).

So yes, that's a significant divergence from 7.x as regards string changes.

catch’s picture

Opened #2563559: Document that contents of render/form arrays passed to alter hooks are unreliable to try to get #19-#23 reflected in the actual hook_form_alter() docs.

Gábor Hojtsy’s picture

Re the string freeze, I raised that concern with others, but should post it here too. It sounds good to give translators two months time to keep up with minor releases, but note that if we want to release Drupal 8 this year, we barely have more than 2 months (we usually rule out December for releases because of major holidays). People definitely need to work more on Drupal 7 to 8 translation updates (vs. minor to minor releases) so we should think about how much time are we going to give them. I raised that question in #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests which proposes to make large sweeping string changes now.

catch’s picture

There was quite a lot of discussion on #2544740: [policy] Consider not supporting core module markup and CSS between minor releases, some of which has been discussed in other places too.

Things we need to add to the doc based on that:

- core templates and CSS (outside of Classy) should be considered @internal.
- the only way we change those is to move the templates to Classy - which means Classy is @api (or at the very least untagged) since you can guarantee the output is the same before or after a move
- Bartik and Seven we should document as @internal - i.e. we'll refactor/improve them and don't use them as a base theme (copy and paste would of course be theoretically OK though)

Crell’s picture

Agreed with #37. Making it clear which CSS/Twig is "safe" to build on and which isn't is crucial for themers. Saying "Classy's safe, the rest is not" seems like a good balance to me.

rickvug’s picture

What about legacy functional code in include files, particularly those in /core/includes, that do not have a OO service equivalent? Would it make sense to have a deprecation notice on each of these functions noting that a 8.x point release may provide a service and that the function itself may be removed before Drupal 9.x?

catch’s picture

@rickvug I don't think we should deprecated things until there's an alternative. Something that's only used internally by the module we can mark @internal though.

catch’s picture

xjm’s picture

Issue summary: View changes

Refinement since Barcelona: Bartik and Seven are considered internal. Needs reference.

catch’s picture

Per #2584419: Profile configuration overrides should be able to override module configuration always we should document that all protected methods are @internal unless either the class is explicitly tagged @api.

Crell’s picture

catch’s picture

Issue tags: +rc target

Should we do the same for protected properties? We either have getters/setters for them, in which case you can use those, or we don't, in which case that's a good sign they should be treated as internal anyway.

Also we don't have many globals left, but for anything we do, we should make access to them @internal per #2183591: Replace global $config with a property on the Settings singleton. @alexpott pointed out we could explicitly document that in globals.php docs?

And probably explicitly document anything that might still be in drupal_static().

We really need to get this firmed up before November 19th, so explicitly marking RC target in case anyone other than core committers is watching that tag. It's a case of finding cases we've forgotten and adding them to the documentation.

In general, it's easy to remove something from that document and treat it as untagged by default as we get into 8.x, but adding something to it is going to be more controversial.

Crell’s picture

+1 to globals being auto-@internal.

For properties, yes, they're even less of an API than protected methods are so should also be @internal-default. I will update the page accordingly.

dawehner’s picture

#2609694: Remove public methods from RouteProvider had an interesting case.
There has been public methods which aren't on an interface. IMHO those should be @internal by default as well. If something exposes an interface, these should be the only non @internal methods.

alexpott’s picture

In IRC @xjm, @chx, and @dawehner and I were discussing #2609694: Remove public methods from RouteProvider. @dawehner said something along the lines of:

xjm: do we support \Drupal::service('router.route_provider')->methodWhichIsNotOnTheInterface() ?

In my mind we don't. If there is a public method on a service but it is not part of any interface the service implements it should be considered @internal - even if it is not marked with it.

[Edit: for clarity]

alexpott’s picture

Also I really like the tables on http://symfony.com/doc/current/contributing/code/bc.html - it makes really clear what can and cannot happen.

xjm’s picture

@alexpott pointed out that http://symfony.com/doc/current/contributing/code/bc.html is a good model, also.

Edit: crosspost.

xjm’s picture

Another thing we discussed regarding #2609694: Remove public methods from RouteProvider is when it is appropriate to mark things as @internal or add to the definition of what is implicitly @internal.

  • I think that once 8.0.0 is tagged, to follow semver, we would not mark anything more @internal unless it were in one of the implicit categories above.
  • @alexpott suggested that we could mark things @internal in a minor and then change or remove them no sooner than a year later (two scheduled minors). I don't really think that fits with semver.
catch’s picture

Adding the docs doesn't stop anyone using it. So just that I think is technically fine in semver, it doesn't say anything about docs at all. Better to stop future people relying on something bad and those already using it can always comment on an issue if they spot it.

For things like this I think we should be prepared to clarify things we forgot, but then if something is actually used after all be prepared to reverse the decision. That might need an '@internal as of' or similar convention so it's clear it was after the fact.

Dropping things after two minors - this is more dodgy. I'd like to see us get active deprecation warnings sorted out before thinking about that too much.

Crell’s picture

Largely agree with catch. Moving from @api->nothing is far more problematic/rude than nothing->@internal, especially for things that are implicitly "not what we are expecting you to use". (The example I used in Austin was RouteFilterInterface should be viewed as an API; the specific classes that core has that implements it should not be; the impact of those, that certain parts of routing.yml will have a certain effect, is part of the API.)

E_USER_DEPRECATED is a tool we've not used to date. Going forward it may be useful in 8.2 or later to note things that we really really plan to move away from, eg, @internal or implicitly so code we know we're going to remove, but haven't yet.

catch’s picture

catch’s picture

#2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways brought up an interesting question via plach.

If we change the logic where a hook is invoked (and the docs don't indicate either way) then there's a possibility of a module breaking based on relying on the previous behaviour.

However, if that behaviour is buggy, then existing modules may also be broken.

So is the specific invocation of hooks an API, or just the docs and signature? I'd say the latter and the former we just only make changes when necessary.

We may need to do soemthing like the following:

- check all contrib modules to see if they'd be broken
- if none are, then make the change with an announcement
- if this turns out to break a custom module, be prepared to roll back the change (at least during beta/release candidate).

catch’s picture

Issue summary: View changes
dawehner’s picture

While the render and form APIs themselves are stable, the exposed data structures used to build and cache pages and forms are not. We will change these structures where necessary to make usability improvements or add features in minor versions.

So do we guarantee that the following code will always be stable throughout the cycle?

$build = [
  '#type' => 'foo',
  '#arg' => 'bar',
];

so basically the interface of render array elements is stable as well? I would strongly vote for it.

The contents of automated tests provided by core modules are never considered part of the API. While the testing framework itself may be treated as an API individual test classes are always internal.

This statement is odd. What do we actually guarantee here? I always believed that tests can break and we are fine with that.

Controllers
Core modules contain many route controllers that are bound to individual routes. These controllers are not part of the API of the module unless specifically marked with an @api tag on the method or class.

IMHO we should do the same for plugins in general, but guarantee at the same time that the options / plugin ID isn't changed, so existing configuration pointing to it, continues to work.

The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release.

Is there a reason we talk about minor releases here? I'd have totally expected to see patch releases here.

catch’s picture

Render elements themselves should be considered neither @api nor @internal for me - so we'd either only make bc changes, or we'd create a new replacement/alternate for bigger changes.

What do we actually guarantee here?

I think this more or less means that we won't just delete WebTestBase etc.

Is there a reason we talk about minor releases here? I'd have totally expected to see patch releases here.

Hmm can you think of a patch-release-eligible issue that needs to make a change like that?

IMHO we should do the same for plugins in general, but guarantee at the same time that the options / plugin ID isn't changed, so existing configuration pointing to it, continues to work.

I'm assuming we'd add additional options and change defaults in minor releases though.

chx’s picture

What about paramconverters? You are not supposed to instantiate them, you are not supposed to call them but perhaps someone extends them, I dunno.

What about non-interface public method on services? Yes it's incredibly bad form to hardwire yourself to a particular implementation but perhaps worth mentioning? And again what happens when someone extends? Are protected methods on service implementation classes API?

catch’s picture

Non-interface public methods I've already added to the doc.

Paramconverters we could add to the same section as controllers. Daniel mentioned plugins above too. Also we haven't mentioned event listeners yet.

Just added https://www.drupal.org/node/2562903/revisions/view/9092526/9100046 to cover all of these.

And again what happens when someone extends?

If they're documented as @internal, but then you extend, then we assume you're prepared to update your code if and when it needs to be. Or if you weren't prepared for that, then it's unfortunate but we're not going to treat it as a bug.

There are going to be things which theoretically might break a contrib or custom module if they're doing something strange, that aren't explicitly covered here by @internal, but also aren't clearly API changes - such as the cache tags HTTP header change or the entity translation hook invocation issue referenced above. Having a BC policy doesn't mean we never break a contrib or custom module, it just means that if we do and it turns out to be something we shouldn't have changed, we treat that as a bug that needs to be fixed - or we can point people to the documentation that said they were relying on something that wasn't considered an API in the first place. We have so many places where people treat application behaviour (as opposed to the API itself) as an API - because that behaviour is usually accessible by one or the other hooks, and it really depends on either very unusual use-cases, or very unusual approaches to solving things which could have been done differently, whether a change to any particular behaviour is going to break something or not.

catch’s picture

Status: Needs work » Needs review

Bumping this back to CNR.

There are a bunch of things in the doc which don't necessarily have broad agreement, however it's much easier to take something overzealous out of the doc after release than it is to add a new thing in.

Also we have a week to remember things that should be in there, but have been forgotten until now, until it gets harder to add them retrospectively.

Crell’s picture

There are many other examples of ParamConverter-like services. RouteFilters and RouteEnhancers come to mind. The interfaces for them are an API, that you can write your own is an API, but I wouldn't call the specific classes we have implemented at the moment an API. There's likely many other examples, too.

How do we want to call those out? Blacklist or whitelist? Or can we make a blanket statement that "in general, classes implementing an interface should not be an API unless otherwise specified"?

catch’s picture

#2350491: Discuss/implement our usage of private services would be the best way to do this for anything that's a service. I don't really see that being done properly within a week.

This is really anything that gets registered to the container by CoreServiceProvider::register(), there might be a way to document that.

Not sure either a whitelist or a blacklist is going to be very good, although whitelist gives us scope to narrow things down later at least if we find a way to be more specific.

Crell’s picture

Well there are many services in core.services.yml that are public, reliable services. There's also many that's not. Without a manual enumeration, though, I'm not sure how to communicate which is which.

Are there any obvious ones we could mark private, even if it's haphazard at the moment, to at least get that started? (Many things can't be marked private because we lazy-load them from a container aware factory anyway, but some could be.)

chx’s picture

> "in general, classes implementing an interface should not be an API unless otherwise specified"

That strikes me as a good policy. What's the point in an interface if the class is the API?

However, we will need then some examples of classes you can reliably extend.

Also we should emphasize it's OK to extend these classes, that's why we do not use private (faugh!) just be aware of potential breakage in a major. Or do we expect to break them in minors too?

dawehner’s picture

That strikes me as a good policy. What's the point in an interface if the class is the API?

Exactly!

Also we should emphasize it's OK to extend these classes, that's why we do not use private (faugh!) just be aware of potential breakage in a major. Or do we expect to break them in minors too?

Well, it could be really hard to guarantee that, especially when "bugs" in a parent class require some changed protected methods. IMHO we should say: Its okay to extend classes and override protected methods but we don't guarantee 100% that they don't break, while still trying the best?

xjm’s picture

Crell’s picture

As a first cut, I think most/all of the classes we want developers to extend are named *Base. Those are the ones where the base class's protected methods should be viewed as an API, and any we don't want to treat as an API should be private in that case.

For any other classes, "extend, but at your own risk". (Those may change in minors.)

catch’s picture

Added a note about locations of procedural functions: https://www.drupal.org/node/2562903/revisions/view/9123136/9145404

catch’s picture

#2629566: Label is not shown for blocks with exposed forms brings up a fun issue.

We want to add protected methods to base classes as API additions.

However adding a new protected method to a class that people extend from is a theoretical bc break - if they happened to have a public or protected method with the same name already, that doesn't match the signature/behaviour.

However, it's a very theoretical bc break, so I think we should be prepared to do this in minor releases.

If something in the development branch for a minor release, and we find a real conflict, then obvously we'd accept bug reports to rename the new method - since then it's not a theoretical break but a real one at that point, but preventing adding methods in general is going to hamstring a lot of reasonable changes.

Crell’s picture

+1 to #70. That also implies we should well-publicize the RC period for minor releases to catch exactly that sort of "not theoretical after all" case and determine if we should adjust accordingly. (There's a couple of oversights in Entity API I've run into recently that should be easily fixed, as long as we're OK with such "theoretical until proven otherwise" cases.)

catch’s picture

Berdir’s picture

From #306662: Add redirect option to site-wide contact forms, on the discussion about adding new methods to entity interfaces or not, quoting my self from there:

The whole discussion around methods, interfaces and BC is quite confusing and IMHO misleading. Here are the facts:

For entities, their properties/keys/fields *are* an API. protected or not, that's irrelevant. Our code expects that they exist, they are exported into YAML (for config) and imported again. We expect that to work.

Saying that leaving out the methods from the interface to preserve BC is pointless if we call them in our code and *expect* them to be there. And as said above, it's the same if we use get('message') or getMessage(). So if we call that unconditionally, then we can just as well add it to the interface. At least then someone that replaced the class will get a clear error what he has to do, instead of things just stop to work.

I can see two ways forward:

a) We document and require that replacing entity classes is supported but they must extend the default class and we can add new methods there in minor releases. You can of course not do that, but then you have to live with the fact that your code might break in minor releases. Just like replacing/using something that's @internal.

b) We add a new interface, ContactFormWithRedirect or something, only implement that in ContactForm and not the existing interface, and wherever we access that value, check for that interface and if the object does not implement that, fall back to the current behavior.

b) is the safe option, but I think a) would be preferrable in many ways. We have many, many issues that are trying to add new features/things to (config) entities, and if we go with b), some of them will have 5 additional interfaces and tons of conditional code in 8.4.

@catch said in IRC that he liked a). So we'd add a paragraph about it to the list of rules above. Anyone against it? (I think most that are working on core on issues like that one will agree with that anyway).

bojanz’s picture

As I said in the other issue, +100 to option a).

A similar issue is #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed, a major bug report that ends up adding 3 new methods to EntityTypeInterface.
That's technically a BC break, but it's also an interface that pretty much nobody outside of core will be implementing, since content/config entities already have so many assumptions baked in. That makes the impact of not breaking BC much larger than the disruption itself.

catch’s picture

For issues like that, I think we need to make sure it's covered by the bc policy. But then if we really think no code will break, we should make the change and just keep an eye out in case we were wrong.

The 6 month cycle is long enough to catch most cases before they get in a tagged release, so there's always the option to roll back.

catch’s picture

@alexpott in the contact form issue:

We document and require that replacing entity classes is supported but they must extend the default class and we can add new methods there in minor releases. You can of course not do that, but then you have to live with the fact that your code might break in minor releases. Just like replacing/using something that's @internal.

But to me this also means that the interfaces are pointless. We should type hint against the entity class because that enforces this.

Well not entirely. If we do A with no modifications, then we let you directly implement the interface, but you're expected to update your code every six months if there's changes.

If we change the type hints to entity classes that would actually prevent people from directly implementing the interface at all. Which is an extra (albeit theoretical again since we don't think anyone is doing this) break beyond adding the methods.

xjm’s picture

Didn't get a chance to read #73 - #76 yet; just adding a separate note here: I don't think the policy yet mentions underscore-prefixed procedural functions, but those should be treated as internal and we should probably just create one patch to explicitly mark them all as such. That way we make the contract clear and can change or remove them as desired in minor releases. #2032893: Deprecate and/or remove _views_file_status() was a simple issue where this came up.

xjm’s picture

Updated the section on CSS, markup, etc. after discussing the current policy and goals with @Cottser:
https://www.drupal.org/node/2562903/revisions/view/9317986/9445199

xjm’s picture

alexpott’s picture

xjm’s picture

Regarding #2484623: Move all JS in modules to a js/ folder:

https://www.drupal.org/core/d8-bc-policy considers the location of procedural code to be internal API. That is, if someone has done:

// Load node.admin.inc from the node module.
  module_load_include('inc', 'node', 'node.admin');

or:

require_once __DIR__ . '/file.field.inc';

We would not break that in a patch release, but we might in a minor.

I guess the question is whether the same applies to assets.

Crell’s picture

Assets should be accessed via libraries only anyway; if we move a JS or CSS file, we'll have to update the corresponding library entry/YAML but the ID of the library shouldn't change, so someone using the API properly should be unaffected. (And if they're bypassing it, well, they're on their own.) So yes, I'd say the same logic applies to assets.

Gábor Hojtsy’s picture

#2518960: Emphasize the most important items on the 'structure' page proposes possibly changing paths under structure wholesale to categorize items better (just like /admin/config/*). Is that something allowed in majors? That is why we have routes right? :)

Crell’s picture

As long as code is properly using route machine names, #83 *should* only impact tests and documentation/screenshots. One way to find out if we did it right... :-)

catch’s picture

@Gábor In terms of code changes, changing a path shouldn't require anything other than updating the route definition.

However, we generally store paths and entity references rather than routes/params in terms of user data, per issues like #2407505: [meta] Finalize the menu links (and other user-entered paths) system and #2351379: [meta] Define, then support exact use cases for link generation and storage (and their related/child issues). This was discussed at length around the first Drupal 8 beta and afterwards.

It might be possible to change paths for admin pages in a minor release, but it will need an update path at least for the link field at a minimum so that shortcuts take it into account.

Note however that this general problem was brought up in 2009 after the original admin/config change at #591682: Config pages are effectively hardcoded to some subcategory of 'admin/config', and there are suggestions in there to make the organisation of configuration pages not depend on the path structure. That issue is still valid as far as I'm concerned.

catch’s picture

#82, that's ignoring stylesheets_remove which is based on path, however it ought to be possible to provide a bc layer for stylesheets remove via mapping old to new paths in the code that processes that.

xjm’s picture

Wim Leers’s picture

@xjm in #81:
We would not break that in a patch release, but we might in a minor.

Great.

@Crell in #82:

Assets should be accessed via libraries only anyway; if we move a JS or CSS file, we'll have to update the corresponding library entry/YAML but the ID of the library shouldn't change, so someone using the API properly should be unaffected.

This is true at the library level, but it's possible to use hook_library_info_alter(), and libraries-override/libraries-extend (and the deprecated-but-still-supported-for-BC stylesheets-remove that @catch mentioned in #86) to perform intra-asset-library manipulation. This is a totally valid use case.

So, I disagree with the "blunt simplicity" in #82, but taking #81 into account, I think the same rule can apply to assets: if we can move PHp files in minor releases but not patch releases, then we should be able to do the same with assets. We can document such changes, and it's trivial for people to update their themes accordingly.

catch’s picture

@Wim in that case we'd be saying the public API is hook_library_info_alter() and the -override -extend functionality, but not anything that gets passed to the hook.

That's more or less what we've send for hook_form_alter() already, but it's going to need to be explciitly documented if we do that, so we've got somewhere to point people to if their stuff breaks.

David_Rothstein’s picture

It might be possible to change paths for admin pages in a minor release, but it will need an update path at least for the link field at a minimum so that shortcuts take it into account.

URL aliases, menu links, etc. also store paths rather than routes - so those would need to be dealt with too.

More generally, URLs are by definition public (and can be linked to from anywhere), so the only way I can really see this working is if a redirect from the old path to the new path is provided.

catch’s picture

Right there's also the potential for someone to use nodes/entities as an internal help document, and link to admin pages from them in text fields, in which case an upgrade path won't help.

I also double checked that it still works to move items around in the admin menu and have them show up in different admin categories, and that bit's fine. Routes don't have to be specially named to show in those categories, that's just how we determine the defaults.

For me an API addition to allow admin routes to specify their category, rather than always using path, is likely to be easier than writing the upgrade path for this.

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.

catch’s picture

dawehner’s picture

Can we expand this section:

Paramconverters, which are never expected to be used directly either as services or value objects, are not considered part of the API. You should not extend from these classes and provide your own implementation instead.

to also include access checkers, which are a way more common usecase than param converters.

Proposed wording:

Paramconverters, access checkers, event subscribers and similar services, which are never expected to be used directly either as services or value objects, are not considered part of the API. You should not extend from these classes and provide your own implementation instead.

xjm’s picture

So it looks like #94 was added to the doc, but I actually was confused when trying to apply it in an issue the other day. Maybe we could link the classes such things would extend/interfaces they would implement/anything that explains what they are?

Edit: I swear that paragraph was in the handbook page the other day, but now it is not. ... Well in any case, +1 for the proposed update, but some links or something would help. :) Like "access checker" could be confused with something very different from what it means.

catch’s picture

I did https://www.drupal.org/node/2562903/revisions/view/9888789/9914139 for now. Not sure where to link to for docs.

dawehner’s picture

Thank you catch!

xjm’s picture

Like just an example of what each thing is would be fine, or symfony docs or whatever. "Access checker" in particular needs context.

catch’s picture

I think we should discuss allowing changes to not-underscore-prefixed functions. This is after reviewing #2340341: Move template_preprocess, _template_preprocess_default_variables into services.

Bc policy has this:

Public methods not specified in an interface
Public methods on a class that aren't on a corresponding interface are considered @internal.

Underscore-prefixed functions and methods
Functions or methods with an underscore prefix (e.g. _some_function()) are considered internal, and may be used to avoid name collisions when backporting protected methods critical or major bug fixes. Extending code should not call these functions or methods directly nor use the underscore prefix for added methods.

This is a bit ambiguous.

For some procedural functions like file_scan_directory(), that's the actual API we expect people to use, so we should obviously provide bc if we refactor to a class method or similar.

However something like template_preprocess() has never been intended to be called by non-theme-system code, it just wasn't underscore prefixed. I think we should allow ourselves to remove functions like that in the case where we'd otherwise have to provide an actual bc layer (potentially with testing). If something is only going to be a one line wrapper it does less harm to just leave it in.

xjm’s picture

https://www.drupal.org/core/d8-bc-policy#bc is applicable:

When we do make changes in either minor or patch releases, we will make reasonable efforts to provide a backward compatibility layer (BC) for anything not marked @internal.

We will take reasonable efforts to not break code that developers may be relying on.

While core developers may change implementation detail code between minor versions when it is necessary to make an improvement, we will make a reasonable effort to minimize these changes.

I'm not keen on the notion of removing something like template_preprocess() on the assumption that it "should" have been internal, because I think that gets us into a dangerous pattern of breaking BC for what appears to be public API (and for globals, no less). It could probably cause real pain for someone out there doing something edgecase.

Instead, if we do consider it internal only, I think we should call it _template_preprocess(), but keep template_preprocess() as a deprecated wrapper. That way people get warnings in their next minor (once we fix that...), and if there is an actual usecase we are more likely to find out in time for a smooth transition to whatever new service and Drupal 9.

I do recognize that it will be tricky to maintain BC layers for complicated (and often brittle) low-level systems like the installer or extension handling, especially if they're still encrusted with procedural code. But if we get to a point that we're struggling with that a lot, that sounds like time for D9.

xjm’s picture

Also, for what it's worth, I think the current BC layer in #2340341: Move template_preprocess, _template_preprocess_default_variables into services looks acceptable to me, including the "why are you using this" deprecation message. :)

xjm’s picture

Ah, and #2340341: Move template_preprocess, _template_preprocess_default_variables into services does include some examples of the types of edgecase code that could break if we remove those sorts of functions (devel_themer for example).

catch’s picture

The bc layer in #2550249-41: [policy, then meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation requires adding a method to an interface, which we explicitly don't allow where there's not a base class - i.e. the patch right now is as much or more of a bc break because of the bc layer than it would be without one. That interface-addition-to-maintain-bc-layer is what prompted me to add the comment here.

If we duplicated the code into a new protected method, that would not be a bc break (just a regular @internal adding a protected method to a class), but then we've 100% duplicated the code (or more likely duplicated it with some minor changes), which then really we ought to have tests for. In this particular case I think that's OK, but there might be situations where that's not viable for bc: even with template_preprocess() there's a drupal_static() reset which could potentially miss (which should definitely count as @internal, but it is a small behaviour change).

The research on that issue since I posted here does show a couple of 7.x contrib modules using it. It'd be interesting to see if any 8.x modules do, but that's more of a reason to provide a bc layer in this case. I don't think it means we shouldn't consider updating the bc policy to allow for cases where we don't want to add one. Something like views_ui_view_preview_section_display_category_links() for example.

It could probably cause real pain for someone out there doing something edgecase.

The current @internal policy already does that to people - in those cases I think we could consider it a valid bug report and roll back the patch or add the bc layer retrospectively. Symfony has done that to us a couple of times and that's been handled similarly (or we've updated code to deal with it if not).

catch’s picture

Just postponed #2795581: Remove unused function statistics_title_list on this issue. It's similar to template_preprocess() in a way, except possibly a better example of where we don't have good options here at the moment:

- procedural function was only ever used as a helper for the statistics block
- when refactoring statistics storage, the block was updated to use a protected method with different logic to account for the storage change
- the patch forgot to move statistics_title_list()
-statistics_title_list() still hardcodes the database schema which could not exist at all or change under its feet, and has no test coverage.
- there's no new API method to replace it.

So we have four choices:
- leave it how it is, wrong, but deprecate it.
- fix it and add tests so it actually works and deprecate it (who's going to do that?)
- make it return [] (especially if the storage class is changed, maybe for everything) and deprecate it
- remove it

drupalcontrib just has the single core usage: http://www.drupalcontrib.org/api/drupal/drupal%21modules%21statistics%21...

timmillwood’s picture

I think it's going to be hard to get a strict policy around this, a lot of it should be committers discretion. If we had a strict policy then statistics_title_list would be marked deprecated, and would break if the statistics storage was changed. In hindsight we should've gone through every function/method in the code and marked them @internal or not.

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.

catch’s picture

catch’s picture

Here's a suggested wording, based on the existing wording for public methods:

Public methods not specified in an interface

Public methods on a class that aren't on a corresponding interface are considered @internal

Procedural functions

Procedural functions in core/includes and core/modules should in general be considered @internal since the majority of core APIs are in services.

If a new service is added to replace a procedural function, then a backwards compatibility layer will be added where possible so that calls to the function continue to work.

However, where procedural functions become 'dead code' (i.e. where usage was removed due to redundancy or because they were moved to a protected method on a class), the function may be removed in minor releases.

dawehner’s picture

I'd be happy with it.

xjm’s picture

I really think #109 is wishful thinking and doesn't reflect the actual state of Drupal 8 at its release. That would be a really big change to the stated definition of the API (as well as expectations about what is API from the entire history of Drupal) and therefore a big BC break. It's difficult enough to manage people's expectations about minor releases without suddenly claiming that all those global procedural functions were supposed to be internal.

I think we could and should add the statement for Drupal 9.

xjm’s picture

In hindsight we should've gone through every function/method in the code and marked them @internal or not.

Well, this was/is supposed to be the issue for that. :) Filed Aug. 2015.

Mile23’s picture

+1 on #109 because #2798273: simpletest_phpunit_configuration_filepath() is dead code (which is postponed on this issue) was discovered while trying to modernize a chunk of simpletest: #2641632: Refactor simpletest's *_phpunit_*() functions etc. to a class

So what will end up happening is that for the modernization we will need to have a BC layer for dead code, and then we'll need to test that the BC layer works to accomplish the behavior of the dead code that no one is calling.

In this case, no big deal. It assembles a string, easy to test. In other cases, a ton of effort might well go into dead code support.

Another solution for dead code would be to mark it as deprecated for the next major release. So in the case of #2798273: simpletest_phpunit_configuration_filepath() is dead code, @deprecated in 8.3.x to be removed before 8.4.0. This gives people a chance to realize they're up the proverbial creek and file an issue, where we can either extend or remove the deprecation. This would mean we can identify code as going-to-be-removed without changing it or needing to add a BC layer right off the bat.

catch’s picture

I really think #109 is wishful thinking and doesn't reflect the actual state of Drupal 8 at its release. That would be a really big change to the stated definition of the API (as well as expectations about what is API from the entire history of Drupal) and therefore a big BC break.

What do you propose we do with statistic_title_list()?

1. Wait for someone to write tests and a bc layer for it? (IMO this is would be incredibly wishful thinking, and a waste of time even if that materialized)

2. Leave it in core possibly working, possibly not working without tests and a bc layer.

3. Remove it as suggested.

4. Something like this:

functions statistics_title_list() {
  trigger_error('statistics_title_list() has been deprecated', E_USER_DEPRECATED);
  return [];
}

(As long as the return type is correct, we haven't broken any API. Won't work for every function but might work for some.).

5. Other options welcome.

#109 isn't suggesting we remove node_load() or file_scan_directory(), it's for code that's in most cases already broken or irrelevant. We already document that hook implementations are @internal. With the exception of user and system module, very few modules have any non-hook/preprocess functions left anyway, but for what's left, I'd prefer a controlled explosion to a time bomb.

catch’s picture

I looked for other examples. There are not that many, most things are already @deprecated wrappers, or would be if we refactored the code into services, or they're hook/preprocess implementations which are already @internal.

menu_local_tabs() is also dead code, although also very short.

tablesort_header() is another example where a bc layer doesn't really make sense (were we to factor it away). That was added in 2004 as a helper. It's in the same file as tablesort_get_sort() which is an actual API function (used in two places in core), but it is not the same thing at all.

We wouldn't want to make that a public method anywhere, it could maybe end up a protected method somewhere, or just in-lined in the one function that calls it, or even built into the twig template.

So the only way to provide 'bc' for that is to leave the duplicated code there (and potentially add test coverage, although since it's not actually an API function it'd have to be a kernel test that builds an array and checks the output of the array afterwards, which we don't have now).

xjm’s picture

@deprecated in 8.3.x to be removed before 8.4.0

The problem with this approach is that it violates semver, unless there was already an explicit expectation that it was internal API as of 8.0.0. And with global procedural functions, I really don't think that expectation was there for them to be internal. I think we need to do a lot more work on defining what is the public surface of our API, and also notify developers with notices or such on dev sites about deprecated and internal API use. However, I do actually think we are stuck with them until D9.

Maybe there is some value in making a distinction for any code that was already dead or broken when D8 shipped. Still, we are going to run into the same problem when we continue to modernize the API and deprecate things. We must have tested, supported BC layers until D9 for SimpleTest, for example.

I'm very opposed to deprecating anything for removal before 9.0.x that is deprecated by any work after the first 8.0.x beta but not already removed before 8.0.0. I do support removing code that was already broken as of 8.0.0. I think @catch's main concern is code that's between these two poles: things that were technically functional as of 8.0.0's release, but not actually intended as public API. For those a case-by-case discussion might be needed, but I think we need to also accept that we are stuck with providing BC until D9. As a rule, nothing should be deprecated for removal in a minor, because then it's not a minor; it's a mis-numbered major.

xjm’s picture

tablesort_header() is another example where a bc layer doesn't really make sense (were we to factor it away). That was added in 2004 as a helper. It's in the same file as tablesort_get_sort() which is an actual API function (used in two places in core), but it is not the same thing at all.

We wouldn't want to make that a public method anywhere, it could maybe end up a protected method somewhere, or just in-lined in the one function that calls it, or even built into the twig template.

So the only way to provide 'bc' for that is to leave the duplicated code there (and potentially add test coverage, although since it's not actually an API function it'd have to be a kernel test that builds an array and checks the output of the array afterwards, which we don't have now).

For me this just means that we have to choose between providing BC/test coverage for the silly not-underscore-named helper for D8, or not doing this hypothetical refactoring in a minor.

I mentioned this to @catch yesterday, but I think we need to make actually defining our public API and explicitly marking things internal a blocker for 9.x. That way at least we can mitigate these problems for the next major, as much as we are stuck with them now.

catch’s picture

I think @catch's main concern is code that's between these two poles: things that were technically functional as of 8.0.0's release, but not actually intended as public API.

Yes exactly. And since we're not explicit in the bc policy about this (but we do say we'll remove public methods on classes, which is the closest thing), I think we should update the document to clarify that not every procedural function is considered API but rather bc is provided best effort. We'd then require bc for anything it's reasonable to require bc for.

If we don't refactor something like tablesort in 8.x, but do in 9.x, then we're introducing an API compatibility between the major branches that's IMO worse than a highly theoretical bc break from removing a function in a minor, i.e.:

1. tablesort gets refactored, with bc for everything, 8.x modules can update to the new API within 8.x, so that they don't need to update for 9.x

2. tablesort gets refactored in 8.x, with bc for everything that's actually an API - same as above with tiny risk that someone was using the not-API for something.

3. Refactor in 9.x only - it's impossible to have code that works with 8.x and 9.x tablesorting.

The big difference is that if you're keeping up, #2 means your module never breaks in either a major or a minor (for this part of the code base) without needing a new major branch for the module. #3 means you get a hard break with 9.x requiring a major branch.

The difference with #1 compared to #2 is the amount of work to provide the bc and test coverage for a theoretical issue - so I think we should default to #1 where it probably-is-an-API, but allow #2 where it definitely-isn't-an-API. If we get it wrong, there's between 2-6 months for someone to notice and ask for roll back.

Mile23’s picture

@xjm: We must have tested, supported BC layers until D9 for SimpleTest, for example.

You might tell that to, eg, #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner (See my comment #28)

@catch: If we don't refactor something like tablesort in 8.x, but do in 9.x, then we're introducing an API compatibility between the major branches that's IMO worse than a highly theoretical bc break from removing a function in a minor

+1

catch’s picture

Another way to do tablesort:

- refactor everything that's an API and add bc layers.

- don't refactor tablesort_header(), but mark it @deprecated, will be removed with no replacement

- open 9.x issue to inline it.

However tablesort_header() might still not be safe, since there are issues like #2701667: Remove template_preprocess_responsive_image() which nuke preprocess hooks and move logic to templates. Nuking a preprocess hook and moving the logic to templates is completely inline with our bc policy.

If we did that with template_preprocess_table(), then it would no longer call tablesort_header(). tablesort_header() once again becomes dead code via a different assassin. In that case we've not even moved the PHP code around, we've replaced PHP code with logic in a Twig template. So whatever way I can come up with, it still ends up being not-an-API by any description of the term.

edit: there's one hint for this case in #2701667: Remove template_preprocess_responsive_image() - it would move the preprocess function for responsive images to stable.theme (since stable is the bc layer). Since the location of procedural code is not guaranteed, then it should be possible to move not only a preprocess function, but a helper for a preprocess function to Stable - that way tablesort_header() ends up living in Stable.

joachim’s picture

dawehner’s picture

One thing we should also mention are HTTP status code/errors messages. Here is a suggestion:

REST HTTP responses are just guaranteed to be stable on 200 responses. This means concrete:

  • Things which have been 500s might become 400s
  • A particular X00 status code might become a different one, to be more compliant with the HTTP spec
  • A particular X00 response body might change its data, to be improved
larowlan’s picture

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.