Problem/Motivation
Drupal 8 core has an established backwards compatibility and internal API policy and a deprecation process, but the @internal
tags have not been documented explicitly everywhere, and some kinds of pseudo-API are not yet covered.
Proposed resolution
Open sub-issues for things not covered. Add children to #2873395: [meta] Add @internal to core classes, methods, properties for things documented on the policy but not in the codebase.
Comments
Comment #2
catchComment #3
catchComment #4
Crell CreditAttribution: Crell at Palantir.net commentedI'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.
Comment #5
catchConstructors are in the issue summary, how is #4 different? Or just the 'unless otherwise noted'?
Comment #6
Crell CreditAttribution: Crell at Palantir.net commentedAh, 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.
Comment #7
Gábor HojtsyDiscussed with @catch:
Comment #8
Crell CreditAttribution: Crell at Palantir.net commentedRe #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.
Comment #9
Gábor Hojtsy@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.
Comment #10
jhodgdonThe 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?
Comment #11
jhodgdonAdding related issue
Comment #12
webchickUnlike @api, my sense is we do need to figure this out before RC1, and @jhodgdon agrees. Adding the tag.
Comment #13
webchickFormally 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.
Comment #14
webchickComment #15
Crell CreditAttribution: Crell at Palantir.net commentedSo 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...
Comment #16
catchComment #17
catchSlightly 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.
Comment #18
jhodgdonDon'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.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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:
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.
Comment #20
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.)
Comment #21
catchThe 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:
@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.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI agree 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).
Comment #23
catchRight 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).
Comment #24
webchickFutzering with this in the handbook atm.
Comment #25
webchickafter 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...
Comment #26
webchickOk, 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.
Comment #27
webchickK, 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.
Comment #28
jhodgdonI 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...
Comment #29
webchickThanks 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 :)).
Comment #30
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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:
Can just be "Controllers and menu callbacks".
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.
Comment #31
webchickSo 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.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThanks 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:
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:
But the new page just says this:
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.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
I agree, this issue is not really related to the RC, but it is important for the Drupal 8 release.
Comment #34
catchSo 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.
Comment #35
catchOpened #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.
Comment #36
Gábor HojtsyRe 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.
Comment #37
catchThere 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)
Comment #38
Crell CreditAttribution: Crell as a volunteer commentedAgreed 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.
Comment #39
rickvug CreditAttribution: rickvug commentedWhat 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?
Comment #40
catch@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.
Comment #41
catchI added a note about HTTP headers following discussion on #2527126: Only send X-Drupal-Cache-Tags and -Contexts headers when developer explicitly enables them and in irc.
https://www.drupal.org/node/2562903/revisions/view/8850573/8999595
Comment #42
xjmRefinement since Barcelona: Bartik and Seven are considered internal. Needs reference.
Comment #43
catchPer #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.
Comment #44
Crell CreditAttribution: Crell at Palantir.net commentedAdded note about protected methods per #43: https://www.drupal.org/node/2562903/revisions/view/8999595/9076054
Comment #45
catchShould 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.
Comment #46
Crell CreditAttribution: Crell at Palantir.net commented+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.
Comment #47
dawehner#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.
Comment #48
alexpottIn IRC @xjm, @chx, and @dawehner and I were discussing #2609694: Remove public methods from RouteProvider. @dawehner said something along the lines of:
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]
Comment #49
alexpottAlso I really like the tables on http://symfony.com/doc/current/contributing/code/bc.html - it makes really clear what can and cannot happen.
Comment #50
xjm@alexpott pointed out that http://symfony.com/doc/current/contributing/code/bc.html is a good model, also.
Edit: crosspost.
Comment #51
xjmAnother 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.
@internal
unless it were in one of the implicit categories above.@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.Comment #52
catchAdding 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.
Comment #53
Crell CreditAttribution: Crell at Palantir.net commentedLargely 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.
Comment #54
catchRe-titled #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases to discuss E_DEPRECATED more.
Comment #55
catch#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).
Comment #56
catchComment #57
dawehnerSo do we guarantee that the following code will always be stable throughout the cycle?
so basically the interface of render array elements is stable as well? I would strongly vote for it.
This statement is odd. What do we actually guarantee here? I always believed that tests can break and we are fine with 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.
Is there a reason we talk about minor releases here? I'd have totally expected to see patch releases here.
Comment #58
catchRender 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.
I think this more or less means that we won't just delete WebTestBase etc.
Hmm can you think of a patch-release-eligible issue that needs to make a change like that?
I'm assuming we'd add additional options and change defaults in minor releases though.
Comment #59
chx CreditAttribution: chx commentedWhat 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?
Comment #60
catchNon-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.
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.
Comment #61
catchBumping 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.
Comment #62
Crell CreditAttribution: Crell at Palantir.net commentedThere 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"?
Comment #63
catch#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.
Comment #64
Crell CreditAttribution: Crell at Palantir.net commentedWell 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.)
Comment #65
chx CreditAttribution: chx commented> "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?Comment #66
dawehnerExactly!
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?
Comment #67
xjmComment #68
Crell CreditAttribution: Crell at Palantir.net commentedAs 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.)
Comment #69
catchAdded a note about locations of procedural functions: https://www.drupal.org/node/2562903/revisions/view/9123136/9145404
Comment #70
catch#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.
Comment #71
Crell CreditAttribution: Crell as a volunteer commented+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.)
Comment #72
catchAdded to the doc: https://www.drupal.org/node/2562903/revisions/view/9145428/9206868
Comment #73
BerdirFrom #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:
@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).
Comment #74
bojanz CreditAttribution: bojanz at Centarro commentedAs 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.
Comment #75
catchFor 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.
Comment #76
catch@alexpott in the contact form issue:
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.
Comment #77
xjmDidn'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 the function _views_file_status() was a simple issue where this came up.
Comment #78
xjmUpdated 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
Comment #79
xjmAlso added https://www.drupal.org/node/2562903/revisions/view/9445199/9445207 for #77.
Comment #80
alexpottThe particular discussion on entity interfaces reflected in #73 to #76 has moved to #2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release as this issue is the generic policy discussion.
Comment #81
xjmRegarding #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:
or:
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.
Comment #82
Crell CreditAttribution: Crell at Palantir.net commentedAssets 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.
Comment #83
Gábor Hojtsy#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? :)
Comment #84
Crell CreditAttribution: Crell at Palantir.net commentedAs 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... :-)
Comment #85
catch@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.
Comment #86
catch#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.
Comment #87
xjm@alexpott posted #2700975: Remove semver promise from testing API.
Comment #88
Wim LeersGreat.
This is true at the library level, but it's possible to use
hook_library_info_alter()
, andlibraries-override
/libraries-extend
(and the deprecated-but-still-supported-for-BCstylesheets-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.
Comment #89
catch@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.
Comment #90
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedURL 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.
Comment #91
catchRight 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.
Comment #93
catchAdded note about preprocess here: https://www.drupal.org/node/2562903/revisions/view/9595485/9684911
Comment #94
dawehnerCan we expand this section:
to also include access checkers, which are a way more common usecase than param converters.
Proposed wording:
Comment #95
xjmPosted #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests.
Comment #96
xjmSo 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.
Comment #97
catchI did https://www.drupal.org/node/2562903/revisions/view/9888789/9914139 for now. Not sure where to link to for docs.
Comment #98
dawehnerThank you catch!
Comment #99
xjmLike just an example of what each thing is would be fine, or symfony docs or whatever. "Access checker" in particular needs context.
Comment #100
catchI 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:
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.
Comment #101
xjmhttps://www.drupal.org/core/d8-bc-policy#bc is applicable:
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 keeptemplate_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.
Comment #102
xjmAlso, 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. :)
Comment #103
xjmAh, 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).
Comment #104
catchThe bc layer in #2550249-41: [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.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).
Comment #105
catchJust 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...
Comment #106
timmillwoodI 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.Comment #108
catchAnother one #2798273: simpletest_phpunit_configuration_filepath() is dead code. Deprecate it..
Comment #109
catchHere's a suggested wording, based on the existing wording for public methods:
Comment #110
dawehnerI'd be happy with it.
Comment #111
xjmI 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.
Comment #112
xjmWell, this was/is supposed to be the issue for that. :) Filed Aug. 2015.
Comment #113
Mile23+1 on #109 because #2798273: simpletest_phpunit_configuration_filepath() is dead code. Deprecate it. (which is postponed on this issue) was discovered while trying to modernize a chunk of simpletest: #2641632: Refactor simpletest's *_phpunit_*() (and junit) functions etc. to a class, deprecate
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. Deprecate it., @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.
Comment #114
catchWhat 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:
(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.
Comment #115
catchI 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).
Comment #116
xjmThe 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.
Comment #117
xjmFor 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.
Comment #118
catchYes 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.
Comment #119
Mile23You might tell that to, eg, #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner (See my comment #28)
+1
Comment #120
catchAnother 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.
Comment #121
joachim CreditAttribution: joachim as a volunteer commented@internal isn't yet part of our coding standards: #2355209: Clearly mark functions that should not be used outside of their project
Comment #122
dawehnerOne thing we should also mention are HTTP status code/errors messages. Here is a suggestion:
Comment #123
larowlanPlease see #2827084-7: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400 - I'm not talking about changes in http response codes, I'm talking about changing the
@throws
signature of a method.Comment #125
xjmJust a note that we are currently working on the "document @internal explicitly" part of this in a new child issue: #2873395: [meta] Add @internal to core classes, methods, properties
Comment #126
xjmA couple things came up reviewing #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal.
So we generally consider the templates and CSS provided by core modules to be
@internal
, and (for stable modules) the stable CSS and templates are provided by Stable and Classy as appropriate. However, we don't really have a best practice way of marking these things internal. For Bartik and Seven, it's just a note in the info file rather than any explicit marking in specific files. Is there a correct way to mark theme code internal?For JavaScript code, @drpal determined that
@internal
isn't really a thing, so he recommended we use@private
instead: http://usejsdoc.org/tags-private.htmlComment #129
Wim LeersNo clarity yet for #2795581: Remove unused function statistics_title_list, nearly 1.5 years after it was postponed on this.
Comment #131
xjmCleaning up stale IS content, and linking the active policies instead.
Comment #132
xjmComment #139
smustgrave CreditAttribution: smustgrave at Mobomo commentedCurious what next steps are for this policy after 3 years?
Comment #140
xjm@smustgrave, we need to finish implementing this across all categories of "automatically internal" things as documented in https://www.drupal.org/about/core/policies/core-change-policies/bc-polic....
Comment #141
xjmBetter status and title. The policy has already been adopted; it's the "meta" part that's still being implemented.