Entity system does not provide an API for retrieving an entity variant that is safe for editing

So, I've been thinking about this a bit and how specifically loading revisions can be super nuanced and potentially even vary under different circumstances. To that end I wrote up a little event subscriber based approach to hopefully help inspire a more wholistic solution.

Features of the approach:

• Event subscribers can be introduced to modify the existing query in a way which seems sensible without needing to invent new interfaces for every new thing that comes along which could change how we load revisions
• Since it's event based, some subscribers can do things like stopping propagation if they believe themselves to be authoritative enough to make that decision
• The priority of events is a super robust solution for getting in front or behind other events.
• Added an get/setRevisionId() method to the event so that individual event subscribers could even go so far as to do their own logic to get a revision id and just set it on the event and call it a day.

Things I might should have done too:

It seems like we could pass an optional string of some sort that denotes the situation in which it's running (kind of like the context parameter for translatable strings). That would mean that layout_builder solutions could be handled differently (if that's desirable) than quickedit. I wasn't sure if that was a step too far, but it'd be a simple, optional addition if we wanted to do it.

I showed this to Tim before posting it, and he mentioned how it's "needlessly bound to the DB, instead of entityquery" which I fully acknowledge. This was just a POC to try to help bootstrap this conversation.

Eclipse

Which entity revision has to be loaded depends on the use case and there is no general answer. Couple of use cases:
- the entity has only default revisions - safe to edit revision is the current default revision
- the entity has default and forward revisions and the module providing the forward revisions allows or disallows the concurrent editing of the default and the forward revisions - a safe to edit revision depends on whether or not the concurrent editing of default and forward revisions is allowed - if not, then the safe to edit revision is the latest forward revision, but if the concurrent editing is allowed then it depends on the context and the changes to be made which revision should be edited - the default or the latest forward revision.
-- even with forward revisions there are more use cases that have to be considered:
--- the forward revisions are per user - the revision to edit depends on the current user
--- the forward revisions are shared among all users - the revision to edit does not depend on the current user
--- a project might introduce additional contexts next to the user based on which a different revision has to be loaded

A (very long) call was held today among some interested folks, the full notes are available here:

https://docs.google.com/document/d/1ku6y-eYkUQXGlstVDoFdWOyHhpYQ6FA35rdC...

A very short summary is: yes, we badly want this, and we need it in 8.6. It's too late in the 8.5 life-cycle to design an API properly.

The end goal is having something exposing a couple of methods allowing API consumers to retrieve the right entity object (correct revision/translations already instantiated) for edit and display without having to worry about the gory details. It would be an object you can safely save or render (respectively) in the current context. A good candidate to host these methods is EntityRepositoryInterface.

It seems clear the we need to come up with a strategy allowing multiple implementers to collectively partecipate in the negotiation process, however hopefully this won't mean they will have to explicitly know each other. Some interesting ideas were mentioned around this subject, but we agreed to have the discussion around the details here.

While the internal details of how to determine the currently active entity revision which is safe for editing are being worked out, we can start to discuss the API method that we want to put out there with a strong message that everyone should be using it instead of direct $storage->load() calls.

Here's a proposal for this new method.

And a few more discussion points:

- should this method handle multiple entities instead of just one? I think that modules like Inline Entity Form can display many entities to edit at once, so it would make sense for them to have a single call for getting all the entities that are being edited.

- should we introduce a similar method for "view" operations? My initial impression is yes, we need the same API when rendering entities, but let's gather some more feedback first.

The second paragraph of the docblock explains the "for editing" part, but not really the "active" part, should we add a paragraph about what an active revision is?

+++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
@@ -8,6 +8,26 @@
+  public function loadActiveEntityForEdit($entity_type_id, $entity_id);

"ForEdit" sounds a little odd, and using a present participle such as "Editing" in a method name doesn't seem right either. Maybe loadActiveEditableEntity?

Should this method handle multiple entities instead of just one? I think that modules like Inline Entity Form can display many entities to edit at once, so it would make sense for them to have a single call for getting all the entities that are being edited.

Maybe we add a load, and a loadMultiple like we already have for entities and revisions?

Should we introduce a similar method for "view" operations? My initial impression is yes, we need the same API when rendering entities, but let's gather some more feedback first.

Yes, even if it's just to go through the thought process (and testing) of what revision should be loaded for viewing and end up deciding the existing load method does this already, or view and edit actually need the same revision (which I don't think will be the case).

What about including into the getter method name a word from the issue title - safe? Active would be depending on some context, but the method in the entity repository is only interested into getting an entity which is safe for editing. I know, that we've agreed on using the word "active", but I think that here it makes sense to use "safe" instead. What about "loadSafeEditable" Or it could be something like ConfigFactory::getEditable() - probably simply loadEditable()?

Nice, so we agree that we also want a corresponding method for view.

I like getEditable() and getViewable() as well. How about the "multiple" part? Should we simply make these two handle it?

I wonder if we need to be explicit that we're loading revisions, getEditableRevision/getViewableRevision.

Question: if you aren't loading an entity to display it or modify it, what are you doing with it? Do we need to deprecate EntityStorageInterface::load? Does the default revision have much meaning, other than being the revision most likely to surface for getViewable in an anonymous request? Should we redefine default to mean exactly that?

Another thought: how do we reconcile that loading the default revision was previously the correct way to display something "viewable"? I imagine in the same way we would treat getEditableRevision. Both ways are okay, one is preferred and you'll be much more compatible with the broader ecosystem if you use the new methods. I think we probably do have to make sure there are no drastic consequences as a result of displaying content from the default revision however.

Lastly, is there any additional context that a future revision negotiator might want to use to figure out the correct revision? Would it make sense to call getEditableRevision('node', 1, 'entity-form'); or getEditableRevision('node', 1, 'rest');. It feels to me like it could be a path for breaking encapsulation, but possibly still worth discussing.

I wonder if we need to be explicit that we're loading revisions, getEditableRevision/getViewableRevision.

In theory EntityStorageInterface::load loads a revision too, but we don't state it. I think we should make it as invisible as possible as to which revision is being loaded. Just that we're loading the editable version of the entity or the viewable version of the entity.

if you aren't loading an entity to display it or modify it, what are you doing with it?

Creating or Deleting it? But I guess those could be classed as editing.

Do we need to deprecate EntityStorageInterface::load?

Would we then change load to call getEditable or getViewable, is the storage calling the repository a little odd?

Does the default revision have much meaning, other than being the revision most likely to surface for getViewable in an anonymous request? Should we redefine default to mean exactly that?

Yes?

Actually the repository will invoke the storage, because most of the logic will land there as the context awareness and the revision branches are concepts, which have to be handled by the storage.

We will be introducing new methods and I don't think we have to deprecate the existing entity retrieval methods from the storages.

The new concepts are pretty complex and therefore I would not force developers to learn them - not in D8 and neither in D9. I would rather leave the current entity retrieval methods as they are in the storage.

Additionally we will have to make the storage load method load by default the safe to edit revision in order to make it possible to keep the current projects as they are, but allow them to enable modules, which are creating revision branches. Thinking about this makes me wonder if we need the methods in the repository?

@effulgentsia pointed me here from #2993557: Allow optional creation of new revision when PATCHing revisionable entities to support autosave functionality via JSON:API., the confusion there would be solved by this. Also, @tim.plunkett++ for creating this great issue summary!

I think this should be classified as critical because it can cause potential data loss if the user is not editing the correct revision whenever edit form is not used.

In an attempt to bring this biggie back to life, here is where I think we stand right now:

• Naming of the methods, as suggested could be: ::getEditable and ::getViewable
• We need to determine how multiples should be handled.
• What is the correct editable entity? From #5 and the earlier meeting, it still need to be decided how implementers can be part of this negotiation process.

I wrote up an issue which possibly means this doesn't have to be a layout builder blocker, due to the nature of the way content moderation already integrates with core: #3004536: Move the Layout Builder UI into an entity form for better integration with other content authoring modules and core features. Might be worth reviewing that if followers of this issue are interested in the compatibility of the two.

Just reading through this, and I can see how this could be a difficult concept to understand, so my only thought right now is that we must be absolutely clear in the approach, in the issue summary and in documentation what the differences are between the various ways of loading entities (assuming we still have different ways of loading entities after this is done).

This now also blocks #2992833 — see #2992833-176: Add a version negotiation to revisionable resource types.

I spent the last few days talking about this issue with @amateescu. We covered several topics:

• Use cases to be addressed
• API overview
• API implementation plan
• Implementation process

What follows is a summary of our discussions. Feedback would be highly appreciated, especially around the use cases, because we need to make sure it's crystal clear what issues this new API is aiming to solve.

Given that amount of new logic the proposed solution implies, and its complexity, I'm marking this as needing release and framework manager feedback. In fact adopting this new API will require a significant change in the way we handle entities.

Use cases

To validate our design we chose to use primarily a fairly advanced, although strictly-real life, use case: editing, in the context of a "stage" workspace, a translated and moderated entity, whose edit form has an hypothetical autosave functionality relying on revisions enabled.

Each entity translation goes through an independent editorial workflow. Once all translations reach an approved state, the workspace is ready for deployment to live. While editing the entity, autosave would trigger the creation of revisions that are only visible to their owner, the active user.

Other use cases we took into consideration, assuming the scenario described above, were:

• the display of a multiple inline entity form widget on the entity form;
• the display of the core Revision UI.

API overview

As discussed in the previous comments, the new API will allow to retrieve the most suitable entity object to populate an entity form and to be displayed with respect to the current context. After discussing this with @amateescu and @tim.plunkett, we agreed that to reduce coupling and make the API easier to test, the caller should be responsible to provide the API implementation information about the current context. For this we would use the core Context API that blocks are already leveraging to implement visibility conditions. This is how the signature would look like:

interface EntityRepositoryInterface {

 /**
  * Retrieves an entity variant suitable to be edited in the specified context.
  *
  * @param \Drupal\Core\Entity\EntityInterface $entity
  *   An entity object.
  * @param \Drupal\Core\Plugin\Context\ContextInterface[] $contexts
  *   An array of objects representing the context the entity will be edited
  *   in.
  *
  * @return \Drupal\Core\Entity\EntityInterface
  *   An entity object variant.
  */
 public function getEditable(EntityInterface $entity, array $contexts);

 // ...

}

Contexts would provide all the required information to perform the revision negotiation. In our reference scenario they would be:

• current workspace
• active language
• current user

As mentioned earlier, the API, to be complete, will have to take care also of entity display. In fact in many cases the editable revision for a particular context will be different from the displayable revision, as access control and fallback logic may all determine different outcomes. For instance, language fallback rules are applied on display, but only a translation matching the active language should be used to populate an entity form.

The API will also have to deal with multiple items both for edit and display. The typical example here would be those mentioned above: a multiple entity reference widget and the core revision UI.

API implementation plan

As mentioned in #5, we need a way to allow multiple implementers to collectively participate in the revision selection process without having to hardcode assumptions about each other.

We propose to organize revisions in a hierarchy that would represent how the implementers handle individual revisions. Each revision would have a reference to its parent along with additional metadata about the context the revision was created in.

Each implementer would be defined via a "revision type" plugin, which might be tied to one (or more?) context. The definition would include a weight that would be used to build a hierarchy between context items, for instance: workspace → active language → user.

The revision graph (think of a tree allowing two parents for each node, technically a directed acyclic graph) represents the revision branches that could be generated when editing an entity in multiple contexts. In our reference scenario an entity could be edited in multiple workspaces, in multiple languages, by multiple users each one triggering their own autosave revisions.

This graph structure will also be used for conflict resolution: when needing to merge revision branches a new merged revision having two parents will be created.

As mentioned above, each revision will have contextual metadata attached, allowing to look up the fittest revision for the specified context. In fact a revision matching exactly the specified context might not exist, so a fallback logic will need to be available. The context hierarchy defined through weights will be used to support that: more and more generic fallback contexts will be built until a suitable revision is found. For instance we may have the following situation:

 Current context:  [ws: stage, lang: it, user: 4]
 Fittest revision: [ws: stage, lang: it]

Assigning a sensible weight to each definition is likely to imply some degree of mutual knowledge among revision types, however theoretically this would not require explicitly coding against each other. It should even be possible to alter definitions to change weights and thus the way revision types play with each other.

OTOH altering revision type definitions and changing context info might make some revision contexts invalid, in which case the fallback logic would have to kick in again with possibly unexpected results.

In our reference scenario, we would have three context-defining revision types: Workspace, Content Translation, Autosave. Content Moderation does not require any special kind of context, so it would not need to affect the revision lookup logic. On the contrary, adopting this new API should allow us to simplify/clean-up the current limitations around revision translation and possibly deprecate the revision_translation_affected flag and the logic around it. This is not an immediate goal, however, as the focus is on stabilizing Workspace.

While the public API mentions variants, since it's dealing with both revisions and translations, under the hood we will just use the revision graph to identify the revision most suitable for the specified context and then instantiate and return the entity translation matching it.

Implementation process

Introducing this API will require a lot of work, so we propose to split it in the following sub-tasks:

• Introduce a new tree field type based on https://www.drupal.org/project/tree_framework: this will finally provide us the foundations needed to build proper entity or revision hierarchies. Despite the name, that project supports direct acyclic graphs as well, so it will be usable in our case. It would also be available to migrate the existing one-off implementations we have in core (parent issue).
• Introduce a custom implementation of the tree field to implement the revision_parent field with the parent reference(s) and the context metadata (parent issue).
• Introduce the API and its implementation based on the revision graph (this issue).
• Introduce multiple support for editable revisions (follow-up issue).
• Add displayable support, both individual and multiple (follow-up issue).

We should probably synchronize the commit of these issues to make sure we don't introduce half-baked solutions in an official core API.

Edit: updated the example image.

Uploading an updated example image depicting also revision translation to be embedded in the previous comment.

A few additional notes from my discussions with @amateescu that people might be interested in.

The proposed plan addresses also the revision translation use case pretty well, via "translation branches", as it allows to tackle the two main issues we had to face while trying to make Content Moderation play well with Content Translation:

• retrieve the correct revision to start from because no parent was available
• avoid conflicts because of no conflict resolution available:
  • prevent changes to untranslatable fields except in the default revision (or default translation) to avoid affecting multiple translations
  • populate the entity form with a “merged” (non-stored) entity having all field values for the current language taken from the “parent” revision and all the other ones from the default revision, so it was always possible to publish it without affecting other languages

Both issues are cleanly addressed by the revision graph, which enables both reliable parent lookup and conflict resolution.

We also briefly discussed whether we should try to generalize the revision graph to deal with variants, that is any possible variation of an entity along any additional axis besides revision and translation, e.g. per-user entity variants. This were my conclusions after some mulling:

• Revisions always aim to get to a single "canonical" representation of the entity (→ non-merged branches are either in-progress or dead branches), other variants not necessarily. Translations have multiple canonical representations, although only one is the "default" one. If we had per-user entity variants, also these would have multiple "canonical" versions, while the "default" one would likely be the anonymous user variant.
• In the example map we have multiple "revision-like" variants, i.e. all aiming for a single canonical representation, which may require merge variants. This (revision) graph could be part of a larger graph (more tree-like) allowing multiple canonical representations of the entity (e.g. per-language, or per-user, or both).
• In our current storage implementation, translations are not variants: all translations are part of every revision, so the graph is actually a revision graph with a language context generating translation branches. Should we ever have a Variant API, translations would be modelled/stored differently and each variant would only hold field values for a single language. In that case we would need the variant graph described above.

👏 👏 👏 👏 @plach++ & @amateescu++!

That's a really incredible summary and it's very exciting!

I had a few miscellaneous thoughts that came up as I was reading it:

1. I assume that the tree field will extend the entity reference field and that it must also support revisions. Do you foresee that the work you do there will overlap with the entity_reference_revisions module? It'd be awesome if this meant revision references made it into core.
2. It seems that the ability to reference a revision is orthogonal to the ability to organize entities/revisions into a DAG. I don't think those capabilities should be wrapped up into the same field type (or field type class at least).
3. This reminds me of an idea/comment from a while ago. I think the issue summary is beyond the scope of what you're proposing here. But if "hierarchy" were introduced as a feature broadly available beyond the revisioning system (and could eventually be reused by taxonomy/book for example), we might be able to kill 2 birds with one stone. The reason I linked that comment was because the "protocol" pattern may be an effective abstraction for separating revision references from data structuring.

For JSON:API, we have a few needs that I think are implicit in this proposal but should probably be made explicit (especially because this proposal has implications beyond the original scope of this issue):

4. An API for getting a revision history
5. An API for getting a revision history for a particular context (for example, only the revision history for Romanian translations)
6. An API for getting the direct parent/descendant revision(s) of a given entity
7. An API for checking access to arbitrary individual revisions. I.e if a user requests /jsonapi/node/article?resource_version=id:42, we should be able to check access to that entity, but also that particular revision (there's not a generic API for this today)
8. I'm not very familiar with the context system (it's also gone through some refactoring recently that I haven't been following). If it's in any way coupled to the Form API, that would make this difficult for API-first use cases.

Not all of that would need/have to come at once, but we should be careful not to preclude any of those capabilities or make them difficult to add in after.

Overall, this is really really impressive. Your use case was really well designed I think.

@gabesullice, #27:

(thanks! :)

1: If I understood @amateescu's plans correctly, the tree field will extend the entity reference field, but won't deal with revisions explicitly. It will be the revision graph-specific customization (a "provider" in the tree field's parlance) to define the revision references, so I don't think the current plan entails bringing ERR to core. @amateescu is this correct?
2: Agreed :)
3: Again, I'll leave a proper reply to @amateescu, but I think the "protocol" approach you are suggesting to implement "horizontal" extensibility without sacrificing strict typing (and IDE autocompletion, I'd say) is interesting and similar to the approach we took in Content Translation with the ContentTranslationMetadataWrapper: that wraps an entity object and provides additional accessors methods around fields added by CT. Does this make sense?

Thanks for making the JSON:API requirements explicit: this exactly the kind of feedback we need! Although I feel these are follow-up material, since this issue is going to be quite big already.

4/5: In this new revision world supporting revision branches, we no longer have a single official revision history, so I'm not sure what that would return. Maybe only default revisions? While discussing the core revision UI with @amateescu, we were thinking that a possible sensible way to populate that page could be to provide a list of all the ancestors of the active (displayable) revision for the current context.
6: Noted :)
7: I'm not sure how a revision access API would look like, maybe just running entity access check on an entity revision object could be enough as a first step. I'm not sure how such an access API would work in the reference scenario we are discussing, but I can see how the things could be related. Thanks for bringing this up.
8: The Context API is a pure API relying on the current request/session, so it should be possible to make it work with REST APIs just fine AFAICT.

Yes this seems like an idea born out of attempts to fix the Content Moderated, Translatable Paragraphs issues.

----

If I understand 4 & 5 correctly, JSON API is seeking to maintain all administrative functionalities available to standard admin scenarios. Therefore the revision history here is similar to looking at the revision history of an entity's revision page / the revision history of a specific entity within a specific workspace. Regardless of the JSON API / standard perspectives we'll need to maintain this key functionality.

7) I can imagine the following use cases that may have you checking the access level of a specific revision:
As a content creator, I want to provide access controls on a specific workspace.
As a content creator, I want to provide access controls on a specific moderation state.

In each case you'll have to look up contextual access information for a specific revision.

This all sounds very exciting, there is possibly a few other core issues that are looking to introduce a core entity hierarchy API, so perhaps this could feed in nicely.

I do a lot of work which involves creating custom content entities, so with these proposed changes it would be great to see if can also reduce the amount of boilerplate code that is needed to create content entities which have revision support.

Thanks for distilling your conversations down into such a succinct summary and making sense of all the discussions that have been taking place. Here are a couple of initial back-of-the-napkin thoughts on the plan:

• Is "editable" the right signature for the method, it seems like the "editable" entity is also the version that users expect to see in all the administration interfaces. For example, in a list of moderated nodes, we need to show users the "draft" version.
• I'm guessing we'll also need a similar new method for "saving", which captures the same contexts.
• Do you think we could introduce this API and "fake" the negotiation by simply baking in some best-guesses based on the installed modules? Might unblock some other criticals while this stuff matures in the background.
• Is the revision tree going to be indexed in such a way that views integration will be possible?
• Is there going to be adequate metadata to upgrade our existing pending revisions to make them usable in this system?

Don't feel the need to respond to all of these points if they're far-off concerns that might distract from getting stuff down now.

@cosmicdreams, #29:

Yes this seems like an idea born out of attempts to fix the Content Moderated, Translatable Paragraphs issues.

Well, that's one use case, and a tricky one, since parallel editing of revisioned Paragraphs means sorting out conflicts between entity revision references, which is definitely not straightforward. I wouldn't say that's the main driver though: to stick with core, we need Layout Builder and Quick Edit to play well with Workspace, Content Moderation and Content Translation (and everything else of course).

[...] Therefore the revision history here is similar to looking at the revision history of an entity's revision page / the revision history of a specific entity within a specific workspace. Regardless of the JSON API / standard perspectives we'll need to maintain this key functionality.

Of course we'll need to keep that working somehow, but what we should clarify is what we are expecting to see/return in that case once we have revision branches. Our suggestion is to display (return) all the ancestors of the revision matching the current (display) context, this way we can always have a straight sequence of revisions going from root to leaf. With this logic, if the default revision matches the current context, we would replicate core's behavior (without CM enabled). If we have a single main branch with pending revisions generated by CM, again we would replicate the current core behavior. So this approach seems to be "backwards-compatible" with respect to what we have now.

And, yes, it definitely seems we should add some kind of advanced access control to our reference scenario, thanks for pointing that out.

@AaronMcHale, #30:

I'm very sensitive to this topic, I'd also like to improve the core entity API so that implementing a custom entity type is just matter of providing an entity class plugin, but this sounds definitely out of scope for this issue and friends.

@Sam152, #31:

[...] Is "editable" the right signature for the method, it seems like the "editable" entity is also the version that users expect to see in all the administration interfaces.

Good point! I think we started referring to this concept as "active" revision at some point in our earlier discussions. Maybe we should stick with that?

I'm guessing we'll also need a similar new method for "saving", which captures the same contexts.

I'm not sure I follow, can you elaborate on this?

Do you think we could introduce this API and "fake" the negotiation by simply baking in some best-guesses based on the installed modules? Might unblock some other criticals while this stuff matures in the background.

Yep, I think this solution is definitely worth exploring. It might help us decouple this issue from the tree/hierarchy work that's definitely not trivial. One possible approach could be to use TranslatableRevisionableStorageInterface::getLatestTranslationAffectedRevisionId() and possibly combine that with some hard-coded assumptions as you were suggesting. I was also thinking that we should probably mark everything as @internal (core-only usage) until we are confident the API and its implementation are mature enough.

Is the revision tree going to be indexed in such a way that views integration will be possible?

Quite possible: Andrei was mentioning he has plans for an alternative storage. It would be great to have something similar to what https://www.drupal.org/project/taxonomy_edge provides.

Is there going to be adequate metadata to upgrade our existing pending revisions to make them usable in this system?

I think those would all end up in the global context, as CM does not require one. An exception could be translated entities: we might be able to re-create translation branches and the related contextual metadata via the RTA flag.

[...] Is "editable" the right signature for the method, it seems like the "editable" entity is also the version that users expect to see in all the administration interfaces.

Good point! I think we started referring to this concept as "active" revision at some point in our earlier discussions. Maybe we should stick with that?

<bikeshed>JSON API has already settled on "working copy" for this concept, as in "the copy on which work is to be done" (we took this from RFC5829).</bikeshed>

I think the "protocol" approach you are suggesting to implement "horizontal" extensibility without sacrificing strict typing (and IDE autocompletion, I'd say) is interesting and similar to the approach we took in Content Translation with the ContentTranslationMetadataWrapper: that wraps an entity object and provides additional accessors methods around fields added by CT. Does this make sense?

100%. I didn't know about ContentTranslationMetadataWrapper! That's great to see that the rough idea has already had some validation. I imagined that if it were more fully fleshed out, "Protocols" would be plugins that could define a set of base field definitions. Entity types would then opt into various protocols via an annotation (and modules could "opt them in" via an alter hook). Once opted in, an implementation of hook_entity_base_field_info could then aggregate the base fields of each protocol implemented and add them all to the entities base fields (with a field name prefix of protocol_ for example).

After more discussions in Slack with @amateescu, @catch, @effulgentsia and @tim.plunkett, we decided to move forward as suggested by @Sam152 in #31 and provide a minimal implementation of the API based on what's currently available in core. I will create a follow-up issue to track the work proposed in #24.

@effulgentsia also asked me whether editable vs displayable should be considered part of the provided context as well, instead of being separate concepts hardcoded in the public API signatures. I didn't have an answer during our call, but the more I think about it, the more I'm convinced that this is an "orthogonal" way of differentiating between variants. I think this ultimately boils down to the concept of "canonical representation" I was mentioning in #26:

• What we were referring to as "displayable" in our previous examples was a variant that can be safely displayed to an unprivileged user: this isn't necessarily the default representation (default revision, default translation) as a non-default translation might be more suitable for the current context (non-default canonical representation).
• What we were referring to as "editable" in our previous examples was a variant that can be safely loaded in an entity form in the current context, however this would also be the variant we would display on the latest revision page provided by Content Moderation.

That said, I think it would make sense to add the following methods to EntityRepositoryInterface:

• ::getActive()
• ::getCanonical()
• ::getActiveMultiple()
• ::getCanonicalMultiple()

Attached you can find a patch with an initial implementation of the solution suggested above.

A couple of notes:

1. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +112,122 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +    // Content language might not be configurable, in which case we need to fall
   +    // back to a configurable language type.
   +    $language_types = $this->languageManager->getLanguageTypes();
   +    $language_type = in_array(LanguageInterface::TYPE_CONTENT, $language_types) ? LanguageInterface::TYPE_CONTENT : reset($language_types);
   +    $info = $this->languageManager->getDefinedLanguageTypesInfo();
   +    $language_type_label = (string) $info[$language_type]['name'];
   

   This is a workaround needed to make the logic work, but I think all defined language types should be returned among available contexts, since it possible to retrieve the negotiated language for any language type from the language manager.

2. +++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
   @@ -69,4 +69,41 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
   +  public function getLatestTranslationAffectedRevision(RevisionableInterface $entity, $langcode);
   

   This is only part of the public API to improve BC in EntityConverter, but it should be removed from the interface before 9.0.0, or even in 8.7.x if release managers are fine with that.

+++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
@@ -69,4 +69,41 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
+   * Retrieves an entity variant suitable to be edited in the specified context.
...
+  public function getActive(EntityInterface $entity, array $contexts);

This is what #2993557: Allow optional creation of new revision when PATCHing revisionable entities to support autosave functionality via JSON:API. should use, regardless of whether Content Moderation is enabled?

@Wim Leers:

Yes, I think the ultimate goal is that whenever we have to modify an entity, either via forms or programmatically, we should adopt this API.

Created #3023194: [PP-2] Add parallel revisioning support as a follow-up issue.

Here is a new iteration implementing the missing bits (canonical variants and multiple support) and incorporating some feedback I got from @amateescu and @pmelab, coming from their work on https://github.com/amateescu/revision_tree : the patch changes the method signatures to accept entity type and ids to avoid having to load all entities in advance when dealing with multiple ones and makes specifying contexts optional, given that in 99% of cases (tests excluded) we need the currently available ones.

The interdiff is not very useful but I'm uploading it anyway for completeness.

The patch should be ready for final reviews.

the patch changes the method signatures to accept entity type and ids to avoid having to load all entities in advance when dealing with multiple ones

👏this is great :)

Thanks!

This should address #42:
1: Fixed
2: That code is definitely suboptimal, see #36.1. However, even if we introduce a reliable way to retrieve content language from contexts, we will still have to compare labels because that's what CurrentLanguageContext uses to distinguish language contexts. OTOH comparing labels should be safe enough, as we are retrieving them from the authoritative source, which takes care of possible translations (that's why we're casting to string).
5: Sorry :)
6: Yep, aside from the fact that in 99% of cases we won't have to specify contexts explicitly.
7: Added a comment about that.

Sorry, the interdiff was wrong, although the patch is correct.

Random test failure, created #3029999: CKEditorIntegrationTest fails randomly for that.

Very nice work overall! There are a couple of non-trivial points below so setting to NW for them.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -372,6 +372,61 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +    @trigger_error('EntityManagerInterface::getActive() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityRepositoryInterface::getActive() instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   ...
   +    @trigger_error('EntityManagerInterface::getActiveMultiple() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityRepositoryInterface::getActiveMultiple() instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   ...
   +    @trigger_error('EntityManagerInterface::getCanonical() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityRepositoryInterface::getCanonical() instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   ...
   +    @trigger_error('EntityManagerInterface::getCanonicalMultiple() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityRepositoryInterface::getCanonicalMultiple() instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   

   8.7.0 -> Drupal 8.7.0 :)

   The same change is needed in the deprecation tests for them.

2. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +129,177 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +        $entity = $this->getLatestTranslationAffectedRevision($entity, $langcode);
   

   Would it be possible to write a "multiple" version of getLatestTranslationAffectedRevision()? Otherwise getActiveMultiple() is kind of pointless :/

3. +++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
   @@ -69,4 +69,84 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
   +   * Retrieves an entity variant suitable for editing in the specified context.
   ...
   +   *   (optional) An array of objects representing the context the entity will
   ...
   +   * Retrieves entity variants suitable for editing in the specified context.
   ...
   +   *   (optional) An array of objects representing the context the entity will
   ...
   +   * Retrieves an entity variant suitable for display in the specified context.
   ...
   +   *   (optional) An array of objects representing the context the entity will
   ...
   +   * Retrieves entity variants suitable for display in the specified context.
   ...
   +   *   (optional) An array of objects representing the context the entity will
   

   context -> contexts :)

4. +++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
   @@ -69,4 +69,84 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
   +   * @param string $entity_id
   ...
   +   * @param string[] $entity_ids
   ...
   +   * @param string $entity_id
   ...
   +   * @param string[] $entity_ids
   

   These should be int|string.

5. +++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
   @@ -69,4 +69,84 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
   +   *   An array of entity identifier.
   

   identifier -> identifiers.

6. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -133,11 +143,7 @@ public function convert($value, $definition, $name, array $defaults) {
   -      // Retrieve the latest revision ID taking translations into account.
   -      $langcode = $this->languageManager()
   -        ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)
   -        ->getId();
   -      $entity = $this->getLatestTranslationAffectedRevision($entity, $langcode);
   +      $entity = $this->entityRepository->getActive($entity_type_id, $entity->id());
   

   Shouldn't we replace the if code block below this with an "else -> $repository->getCanonical()"?

   If we do that, we will lose the ability to pass ['operation' => 'entity_upcast'] to EntityRepositoryInterface::getTranslationFromContext(). Could that be considered a slight BC break, given that ConfigurableLanguageManager::getFallbackCandidates() fires an alter hook which contains information about the operation context?

7. +++ b/core/modules/content_moderation/tests/src/Kernel/EntityRevisionConverterTest.php
   @@ -29,10 +30,15 @@ class EntityRevisionConverterTest extends KernelTestBase {
   +    $user = User::create(['uid' => 1, 'name' => $this->randomString()]);
   

   Is the usage of UID 1 intentional? If not, it would be nicer to use another UID which doesn't have any special meaning.

8. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRepositoryTest.php
   @@ -0,0 +1,247 @@
   +    $user = User::create(['uid' => 1, 'name' => $this->randomString()]);
   
   +++ b/core/tests/Drupal/KernelTests/Core/ParamConverter/EntityConverterLatestRevisionTest.php
   @@ -48,6 +49,10 @@ protected function setUp() {
   +    $user = User::create(['uid' => 1, 'name' => $this->randomString()]);
   

   Same here.

9. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityRepositoryTest.php
   @@ -31,6 +32,13 @@ class EntityRepositoryTest extends UnitTestCase {
   +   * The language manager.
   ...
   +  protected $contextRepository;
   

   We shouldn't lie about what the variable holds :)

Also, crediting @pmelab for his help with shaping this new API.

This should address #48 and #42.2 (the interdiff is rather big, individual commits available in the sandbox):

5c892c68d5 DEV 2942907: Fixed deprecation messages.
436a5cb281 DEV 2942907: Fixed PHP docs.
708fcd70ba DEV 2942907: Fixed test account IDs.
fce400b2c3 DEV 2942907: Added missing @todo.
d0c6e31bd6 DEV 2942907: Added "variant" suffix to the new methods.
b6035cded5 DEV 2942907: Added fallback to canonical variant in EntityConverter.
04d28115de DEV 2942907: Fixed kernel test failures.
ff356c2635 DEV 2942907: Fixed non-existing entity handling.
0f3f5c5b8e DEV 2942907: Fixed entity type ID check.
106bb1231a DEV 2942907: Removed BC-breaking optimization.
998cd5cc92 DEV 2942907: Improved content language retrieval.
61b003950d (HEAD -> 8.x-entity-editable_api-2942907-plach) DEV 2942907: Fixed issue with current user context

I discussed #48.2 with @amateescu and we agreed to work on that in a follow-up if for any reason #3023194: [PP-2] Add parallel revisioning support cannot happen in time: #3031082: Optimize EntityRepositoryInterface::getActiveMultiple().

This patch also includes some changes to the method names to (hopefully) improved DX: we are adding a Variant suffix to clarify that we are dealing with entity variants, which is the official name we use to refer generically to entity revisions and/or translations.

I was attempting to clarify the documentation linked above when I realized that the distinction between "active" and "canonical" I proposed in #35 does not cover the full picture. I think "editable" vs "displayable" are two modes that apply orthogonally to both "canonical" and "active" variants:

• The "active" variant corresponds to what we currently label as "latest" revision, although with Workspaces and parallel revisioning we will have a latest revision for each branch (branch HEAD in git). On top of that, if we edit the active revision (via form or REST), we need to pick an existing translation, or create a new one matching the current language, whereas if we display the active revision language fallback rule may apply and the translation displayed might not match the current language.
• By far the most common use case for canonical variants is display, however also a canonical variant can be editable, at least theoretically: in the reference use case of the typo-fixer, the typo will be fixed starting off the canonical variant. In practice normally the variant will be loaded in the live workspace, so it will be the active one in that context.

In this light @effulgentsia's idea to make editable vs displayable part of the contextual information makes much more sense to me: even if edit and display are well defined operations and have distinct logic applying to each one in our implementation, this might not be the case with alternative implementations of the entity repository. Contrib/custom code could provide alternative operations that make sense for their specific implementation.

All that said, I think the new API should expose three methods (and their "multiple" counterparts):

• ::getCanonicalVariant(), assumes the "view" operation. Optionally supports the "edit" operation to skip language fallback rules and return an existing translation (or a new one) matching the current context.
• ::getActiveVariant(), assumes the "edit" operation by default, and returns an existing translation (or a new one) matching the current context. Optionally supports the "view" operation context to apply language fallback rules.
• ::getDisplayableVariant(), thin wrapper around ::getActiveVariant() hardcoding the "view" operation.

Thoughts?

TODOs for myself:

1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -125,32 +136,46 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lan
   +      $contexts = $contexts_repository->getAvailableContexts();
   

   We likely need a way to statically cache this, it doesn't feel ok to perform it every time we perform a param conversion.

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -161,39 +186,25 @@ public function convert($value, $definition, $name, array $defaults) {
   +      new Context(new ContextDefinition($data_type, $info[LanguageInterface::TYPE_INTERFACE]['name']), $langcode),
   +      new Context(new ContextDefinition($data_type, $info[LanguageInterface::TYPE_CONTENT]['name']), $langcode),
   

   Missing keys and related test coverage.

3. +++ b/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentCacheabilityBubblingTest.php
   @@ -33,7 +36,7 @@ protected function setUp() {
   +    $this->setUpCurrentUser(['uid' => 0]);
   

   Is this still needed?

4. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -89,4 +99,64 @@ public function testClearCachedDefinitions() {
   +  public function testGetActive() {
   ...
   +  public function testGetActiveMultiple() {
   ...
   +  public function testGetCanonical() {
   ...
   +  public function testGetCanonicalMultiple() {
   

   Update method names.

5. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -168,26 +170,33 @@ public function getActiveMultiple($entity_type_id, array $entity_ids, array $con
   +    $key = '@paramconverter.entity:legacy_context_operation';
   

   This should be @entity.repository:legacy_context_operation.

6. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -212,16 +221,10 @@ protected function getContentLanguageFromContexts(array $contexts = NULL) {
   +        $langcode = $contexts[$context_id]->getContextData()->getValue()->getId();
   

   ::getContextValue()

+1

From #49:

[...] I think the new API should expose three methods (and their "multiple" counterparts):

• ::getCanonicalVariant(), assumes the "view" operation. Optionally supports the "edit" operation to skip language fallback rules and return an existing translation (or a new one) matching the current context.
• ::getActiveVariant(), assumes the "edit" operation by default, and returns an existing translation (or a new one) matching the current context. Optionally supports the "view" operation context to apply language fallback rules.
• ::getDisplayableVariant(), thin wrapper around ::getActiveVariant() hardcoding the "view" operation.

Thoughts?

I discussed this topic at length with @amateescu: our conclusions were that maybe we don't need to differentiate between "edit" and "view" after all.

The two main areas we covered were language fallback and access control:

• So far I always mentioned language fallback as a differentiator between "display" and "edit" contexts, however this is an outdated approach, that was valid in D7 but has been obsoleted in D8 by the translation negotiation logic we have now. In D7 we applied language fallback at field level, whereas in D8 we instantiate a whole translation based on the language fallback rules. This provides a more consistent UX, in fact in the entity form we load the same entity variant that is displayed on entity view, which is the normal/expected behavior on monolingual sites.
• Access control was trickier to sort out and we did not reach a final conclusion, so feedback would be welcome especially on this: we tried to figure out whether any kind of access control logic would have to be baked into the revision negotiation process, because this in turn would require us to specify the operation (edit/view) among the contextual info. We used as reference the use cases suggested in #29:
  1. As a content creator, I want to provide access controls on a specific workspace.
  2. As a content creator, I want to provide access controls on a specific moderation state.
  3. Additionally, we considered the case of a multilingual site with dedicated translation teams for each language, having edit access only to the translations matching their language.

  In all cases, assuming all users have edit access on an entity, there may be revisions they have access to and other they don't have access to. For access logic to be required to be part of the negotiation process, though, we would need some kind of access fallback rule: "you don't have access to this revision, so here is another one you can manipulate". However, this is never required in the examples above and keeping negotiation and access control separate feels cleaner from an architectural POV.

  More specifically, considering the use cases above:

  1. A module would implement access control to prevent users from accessing specific workspaces, this in turn would prevent users from changing active workspace, thus negotiation would rely on contextual information respecting the access control rules. Of course the module would also be responsible for denying access to revisions associated with protected workspaces.
  2. A module would implement access control preventing users from having access to a revision in a certain moderation state. There is no negotiation involved here, because CM only works sequentially or as part of a workspace, which would bring us in the preceding case. In the sequential case there's no fallback possible: either you have access to the latest revision or you don't.
  3. Again, with revision translation access control there would be no fallback involved: if you don't have access to the revision translations in a particular language, there is no fallback revision that it would make sense for you to handle.

  The common theme seems to be that if you don't have access to the active revision, there is no possible access control-driven fallback logic.

To sum up, if people agree with our findings above, there's no need to differentiate between "edit" and "display" contexts, and we can just rely on the canonical variant (default revision) vs active variant (latest revision) split.

Edit: fixed typos/grammar.

Great work here!

1. To sum up, if people agree with our findings above, there's no need to differentiate between "edit" and "display" contexts, and we can just rely on the canonical variant (default revision) vs active variant (latest revision) split.

   Totally agree!

2. I also would vote for no baked in access control.

   In layout builder we separate our access to control the layout for an entity from access to editing the entity.
   i.e, you can have access to edit the layout with edit access to the entity.

   so we would not want getActive() to not return back anything if the current user cannot edit the revision. I think it should be the callers responsibility to check access.

3. In #2957425-239: Allow the inline creation of non-reusable Custom Blocks in the layout builder we made the assumption that $load_entity_revision->access() would check access for the revision but it does not. This is an existing issue and if we could solve in separate issue that would be great.

   Because if we baked into this issue that would still not solve the problem of
   $this->entityTypeManager->getStorage('node')->loadRevision('2')->access()
   not actually solving checking access for the revision. maybe ultimately we would need
   checkRevisonAccess() but we should have access checking for any revision not just the `active` or `display` variants.

This should address #51.

More static caching:

drupalci fail

I'm afraid we have to use proper static caching here.

Minor clean-up

1. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -26,6 +27,20 @@ class EntityRepository implements EntityRepositoryInterface {
   +   * All the available contexts.
   +   *
   +   * @var \Drupal\Core\Plugin\Context\ContextInterface[]
   +   */
   +  protected $availableContexts;
   

   We're adding this static cache in two places already in the patch, shouldn't we move it to the context repository directly?

2. +++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
   @@ -69,4 +69,88 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
   +  public function getActiveVariant($entity_type_id, $entity_id, array $contexts = NULL);
   ...
   +  public function getCanonicalVariant($entity_type_id, $entity_id, array $contexts = NULL);
   

   I'm not a big fan at all of the "variant" suffix, I think it makes the method names look scarier than they should be.

   Thinking about this for a while, I'm warming up to the proposal of using "working copy" instead of "active". It's a terminology that's already established in a few places outside of Drupal, and I think it conveys the message for "give me an instance of this object that is ready for editing" quite well.

3. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -125,32 +143,59 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lan
   +      // @todo At the moment we do not need the current user context, which is
   +      //   triggering some test failures. We can remove these lines once
   +      //   https://www.drupal.org/node/2934192 is fixed.
   

   Does this mean that we can remove all the test changes related to the current user (including the trait) from this patch?

1: Unfortunately both EntityConverter and EntityRepository may retrieve contexts independently, meaning on different execution paths, so we cannot share a static cache unless the context repository itself does it. Btw, I cannot locate that hunk in the current code, was it from a previous patch?
2: I have to think about this, I'll come back to it with my thoughts. Of course method names are still up for debate as anything else :)
3: Nope, the current user stuff is now needed because the current user context relies on at least one user existing in the database, it's unrelated to that @todo.

so we cannot share a static cache unless the context repository itself does it.

That's what I proposed :)

Btw, I cannot locate that hunk in the current code, was it from a previous patch?

Very likely, I think I added the comment in an old tab. That's also the reason for the status change..

That's what I proposed :)

D'oh, reading whole comments would help from to time, sorry.

I didn't go that way because: 1) I'm not sure that's desired 2) it might be tricky to do safely, because current contexts are global to the request, but I'm not sure that's true for any context, so we'd need a separate issue to address that carefully.

It would be great to have Tim Plunkett's feedback on this.

This should fix test failures.

Better fix

`editable` and `viewable` do not suggest a single variant, let alone a very particular single variant. `activeVariant` and `canonicalVariant` have more of that flavour.

That documentation is outdated and wrong: the split is no longer between editable and viewable, but between canonical variant (default revision + translation negotiation) and active variant (latest revision + translation negotiation). See #53 and previous comments for details.

The names varies on the revision state, why not use latest and default instead? getDefaultVariant and getLatestVariant.

Needs reroll I think because of #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods

Not sure how useful this is, but when working with Entity Types canonical refers to the route/controller that is used when the user is viewing the Entity, e.g. entity.node.canonical. So there is precedent for the use of canonical but it's important that use of the term is accurate to mean the revision the user is currently viewing, as we shouldn't cause any unmercenary confusion.

I've only loosely been following this discussion so not sure how useful my input is but thought I'd mention it either way.

Thanks
-Aaron

This updates the documentation to reflect what we was described in #53 and should address #71.

I discussed static caching with @tim.plunkett and it seems that would be better addressed in a separate issue. I'll ping @catch about this.

@amateescu, #62.2: I thought about the proposal of using "working copy" terminology a bit. It would work for me if people feel that's preferable, personally I'd still prefer "active variant" because it seems that the examples you mentioned only take revisions into account, whereas here we are trying to reinforce the concept of variant, i.e. something that may vary based on multiple axes. However I realize this might be subjective :)

@AaronMcHale, #77: that was definitely the main reason why we adopted the "canonical" term :)

I'm not a big fan at all of the "variant" suffix, I think it makes the method names look scarier than they should be. Thinking about this for a while, I'm warming up to the proposal of using "working copy" instead of "active". It's a terminology that's already established in a few places outside of Drupal, and I think it conveys the message for "give me an instance of this object that is ready for editing" quite well.

Same here. However, there's a bit of difficulty in mapping our terminology to RFC 5829. I might be wrong, but as far as I can tell, their use of "working copy" isn't compatible with versioning of the working copies. In other words, the viewable resource is the one with a version history, including a "latest version" (which would correspond with our "default" variant), but the "working copy" is a separate resource which then becomes the "latest version" of the original resource when it is "checked in". I suppose nothing in that spec prevents the separate "working copy" resource from also being a versionable resource with a "latest version" (which would correspond with our "active" variant), but my reading of that RFC is that that is a different model than what we have in Drupal, where we maintain a version history for the not-published (active) copies as well as for the published (default) copies as a single history of a single entity.

I think there might be a way to adapt the language to clarify this more, but still retain some correspondence to that RFC.

How about:

• getCurrentCanonical() (because there are multiple versions that have been the canonical one at some point, and we're interested in the one that's canonical now)
• getLatestWorkingCopy() (combining the terminology of "latest" and "working copy" to clarify that our working copies are themselves versioned)

I'm totally open to these suggestions being rejected or further improved.

I'd still prefer "active variant" because it seems that the examples you mentioned only take revisions into account, whereas here we are trying to reinforce the concept of variant, i.e. something that may vary based on multiple axes.

In my opinion, there's nothing in "getCurrentCanonical" or "getLatestWorkingCopy" that implies a lack of axes such as language. The "current canonical" can be language-dependent as can the "latest working copy".

I spoke with @effulgentsia yesterday about #53 and the overall architecture and he was onboard with the route we took so far. We went through several use cases, but the main outcome of our discussion was that the following statement from #53 is actually correct:

The common theme seems to be that if you don't have access to the active revision, there is no possible access control-driven fallback logic.

The reason is that contextual information acts as an identifier itself, thus, even if we may apply fallback rules to identify a revision matching the current context, once we identify a matching context, that's the active revision and there's no further fallback possible, because a specific (revision) resource is unequivocally identified by the specified contextual information.

[...] I think there might be a way to adapt the language to clarify this more, but still retain some correspondence to that RFC.

If we can achieve that it's great, but honestly I don't think it's a requirement. I wouldn't do that at the cost of making the method names less clear or descriptive.

How about:

• getCurrentCanonical() (because there are multiple versions that have been the canonical one at some point, and we're interested in the one that's canonical now)
• getLatestWorkingCopy() (combining the terminology of "latest" and "working copy" to clarify that our working copies are themselves versioned)

So, I'm not sure about getCurrentCanonical(): personally I have the same issues with it that I had with getCanonical(), I can't help but thinking "get canonical what?". On top of that, I'm not sure adding "current" really buys anything in this context, because the API doesn't let you retrieve a previous canonical version. Unless we add that, this sounds like cognitive overhead to me.

getLatestWorkingCopy() sounds better to me, although I'm a bit concerned about the "copy" terminology: here we are not dealing with a copy of the original/canonical resource, but with a variant of the same resource. I think that "copy" is potentially confusing from this POV.

Were I to choose, I'd still go with active/canonical + the variant terminology, since "variant" is the official term we introduced to refer generically to translations and revisions in our documentation. However I think it's more important that people that aren't deeply involved with this issue provide their feedback, since they are those who will benefit/suffer the most from this choice. For that reason, I think I'm the last person that should be making this choice :)

Btw, "active" is consistent with the "active language" and "active translation" terminology we use in the Entity Translation API.

I spoke with @tim.plunkett and @catch (separately) about statically caching available contexts: Tim suggested to address that in a follow-up and Nat confirmed that a major followup is acceptable because param conversion is only triggered when rendering tabs, so the performance hit is more or less constant. Additionally, this is not an issue introduced by this patch, rather by the way we do tabs/contextual links.

With this change I think the code reviews above are all addressed and we just need to find consensus on the method names.

Here's a first review, I read the IS (which I guess isn't up to date or at least it's very short) but almost none of the comments, I did follow this issue for a while though but usually just *very* quickly skimmed through the notifications.

One thing I don't understand yet is how the contexts are playing into this, so it would be very helpful to have that explained in the IS. It likely is already in some comments, but this issue has a lot of discussion and longs comments.

I finally got around to do a review because I've been working on #2988331: Content translation moderation quite a lot recently, and it is very closely related to this. It's about finding the right revision/translation, both in a hook and a controller and displaying all kinds of information whether it is the default revision or not, if there are pending revisions, if the current entity object is a pending revision and so on. That was surprisingly hard (this issue is critical for a reason) I learned a few things in that issue, so I wanted to share it here, I don't think this issue can/should address all of it, but maybe a few things could be improved or we can create follow-ups.

* \Drupal\content_moderation\ModerationInformation has several methods that seem weird/wrong and we should consider to deprecate them. Most obvious is isLatestRevision(), which currently doesn't work at all with translations. Another is isLiveRevision(), the concept of "live" is very strangely defined there, it "live" when it is the latest *and* default revision and is published. That doesn't make any sense to me and the method does in fact seem to be completely unused in core. Unsurprisingly. I guess they should have been deprecated already when $entity->isLatestRevision() and $entity->isLatestTranslationAffectedRevision() was added. Because contrib modules like moderation_sidebar still use them.
* getLatestTranslationAffectedRevisionId() can return an old non-default revision if the another translation was saved last and then isDefaultRevision() returns false but it is not a pending revision. See the changes in ModerationSidebarController::sideBar(). To reproduce, create an EN entity, then add a DE translation as published, then call getLatestTranslationAffectedRevisionId($id, 'en'), which will still return the revision that EN was, well, last affected. This seems rather confusing and I don't think some of the comments that are in this patch are aware of that.
* I also had troubles to figure out if the currently entity is a pending revision, bu I forgot about \Drupal\Core\Entity\ContentEntityBase::isLatestTranslationAffectedRevision(), that should allow me to simplify my patch a bit.

1. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +129,193 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +      // Retrieve the fittest translation, if needed.
   +      if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
   +        $entity = $entity->hasTranslation($langcode) ? $entity->getTranslation($langcode) : $entity->getUntranslated();
   +      }
   

   isn't the loaded entity always the default translation?

2. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +129,193 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +    // Content language might not be configurable, in which case we need to fall
   +    // back to a configurable language type.
   +    foreach ([LanguageInterface::TYPE_CONTENT, LanguageInterface::TYPE_INTERFACE] as $language_type) {
   +      $context_id = '@language.current_language_context:' . $language_type;
   +      if (isset($contexts[$context_id])) {
   +        $langcode = $contexts[$context_id]->getContextValue()->getId();
   +        break;
   

   configurable or configured?

3. +++ b/core/lib/Drupal/Core/Entity/EntityRepositoryInterface.php
   @@ -69,4 +69,116 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
   +   *
   +   * @internal This is an experimental API. It will be published once it is
   +   *   deemed mature enough. In the meantime, adopters should be prepared to
   +   *   update their code to accommodate API changes.
   +   */
   +  public function getMultipleActiveVariants($entity_type_id, array $entity_ids, array $contexts = NULL);
   

   I'm not sure I understand how a non-internal interface can have internal methods. Changing anything would break all implementations of it?

4. +++ b/core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php
   @@ -50,13 +48,14 @@ class AdminPathConfigEntityConverter extends EntityConverter {
   +   * @param null $removed_parameter
   +   *   (deprecated) The language_manager service is deprecated in Drupal 8.7.0
   +   *   and will be removed before Drupal 9.0.0.
       */
   -  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, AdminContext $admin_context, LanguageManagerInterface $language_manager = NULL, EntityRepositoryInterface $entity_repository = NULL) {
   -    parent::__construct($entity_type_manager, $language_manager, $entity_repository);
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, AdminContext $admin_context, $entity_repository = NULL, $removed_parameter = NULL) {
   +    parent::__construct($entity_type_manager, $entity_repository, $removed_parameter);
    
   

   What exactly is the removed parameter for here? The language manager was not at that position, so AFAICS, we could just remove it, including the documentation? Subclasses can have additional constructor arguments and also pass them to the parent: https://3v4l.org/gpS9D.

5. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -88,36 +93,42 @@ class EntityConverter implements ParamConverterInterface {
        }
        else {
   -      @trigger_error('The entity.repository service must be passed to EntityConverter::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
   -      $this->entityRepository = \Drupal::service('entity.repository');
   +      if ($entity_repository instanceof LanguageManagerInterface) {
   +        @trigger_error('The language_manager service is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. See https://www.drupal.org/node/2938929.', E_USER_DEPRECATED);
   +      }
   

   the language manager service is most certainly not deprecated, only passing it to the constructor is :)

   I'm not sure we need to be that fancy with the deprecation messages, this is quite a lot of code for BC.

6. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -125,32 +136,46 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lan
   -    $entity = $storage->load($value);
   +    // Check that a valid entity type was specified.
   +    try {
   +      $this->entityTypeManager->getDefinition($entity_type_id);
   +    }
   +    catch (PluginNotFoundException $e) {
   +      // Prior to 8.7.0, this class would throw the following exception when an
   +      // an invalid entity type was specified.
   +      throw new InvalidPluginDefinitionException($e);
   +    }
    
   

   Why don't we use if ($this->enityTypeManager->hasDefinition() { throw exception) instead of a try/catch-rethrow?

   As comments usually don't explain the history of something, I'm not quite sure what it is trying to tell me. As far as I see, this can not be a temporary BC thing, so it still and will continue to throw that type of exception indefinitely (seems like the fact that it throws this exception and not the other is pretty random and only because we called getStorage() first).

7. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -275,13 +289,7 @@ protected function getEntityTypeFromDefaults($definition, $name, array $defaults
       * @internal
       */
      protected function languageManager() {
   -    if (!isset($this->languageManager)) {
   -      $this->languageManager = \Drupal::languageManager();
   -      // @todo Turn this into a proper error (E_USER_ERROR) in
   -      //   https://www.drupal.org/node/2938929.
   -      @trigger_error('The language manager parameter has been added to EntityConverter since version 8.5.0 and will be made required in version 9.0.0 when requesting the latest translation-affected revision of an entity.', E_USER_DEPRECATED);
   -    }
   -    return $this->languageManager;
   +    return $this->__get('languageManager');
      }
    
   

   this is an explicitly tagged internal protected method, can we just drop it?

   In #2244513-186: Move the unmanaged file APIs to the file_system service (file.inc), we just added such a method as private to prevent it from being used.

8. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -142,4 +142,64 @@ public function testLoadEntityByConfigTarget() {
        $this->assertInstanceOf(EntityInterface::class, $this->entityManager->loadEntityByConfigTarget('config_test', 'test'));
      }
    
   +  /**
   +   * Tests the getActiveVariant() method.
   +   *
   +   * @covers ::getActiveVariant
   +   *
   +   * @expectedDeprecation EntityManagerInterface::getActiveVariant() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityRepositoryInterface::getActive() instead. See https://www.drupal.org/node/2549139.
   +   */
   +  public function testGetActiveVariant() {
   +    $entity_type_id = 'entity_test';
   

   these methods are definitely going to conflict a few times unless it gets in very soon if we don't get them in soon as I'm continously adding more deprecated tests here as I'm adding @trigger_error() to more and more methods.

   Maybe add them further up in the class, between some other methods to limit that risk? not pretty, but it's just a BC test.

Thanks for the review, @Berdir!

The IS is definitely outdated, since we didn't find consensus on the overall strategy until very recently. I will update it ASAP, however the most important thing you should be aware of in the meantime is that the implementation of the API in the current patch is meant to be replaced by the more complete one outlined in #3023194: [PP-2] Add parallel revisioning support. The IS over there is up-to-date.

Btw, since you are not familiar with the previous discussions yet, any early feedback on the method names?

This should address the review above, a few replies follow:

@tim.plunkett, #85:

1: Fixed
2: Fixed. When you say "should" are you referring to the PEAR coding standards? I couldn't find anything relating to ternaries in ours.
3: Fixed. I prefer multiple lines when a single line risks not to fit a normal-sized viewport.
4: Not strictly, I thought it would still be good to have it since it's used in two places and makes the logic easier to override for subclasses. Not feeling strongly about this, I can remove it if it's undesired.
5: Fixed
6: Fixed
7: Removed that part entirely: I realized that EntityConverterTest::testConvertWithInvalidEntityType was expecting the wrong exception, because the previous code was calling ETM::getStorage() and that does throw PluginNotFoundException when the entity type does not exist.
8-9: Added a comment to make things more explicit.
10: Good point, I need to remember about this when writing the CR.

@Berdir, #86:

1: Yep, which reminded me that we were planning to perform translation negotiation also when retrieving the active variant. Fixed that and added the related test coverage.
2: Configurable. If I'm not mistaken ConfigurableLanguageManager::getLanguageTypes() returns all configurable languages, regardless of their configuration being actually customized.
3: Very good point. The idea was to be able to iterate on this, but clearly we can't do that if implementors break. What about adding a separate interface only the core EntityRepository implements?
4: That's the only way I found to keep BC: the current code supports both passing two parameters (correct) and three parameters (deprecated). I agree the PHP doc and the variable name are a bit confusing but I don't know how else to document this.
5: Fixed the message. If I'm not mistaken most of the logic there is to support both 2 and 3 params and retain BC, it's not about being fancy :)
6: See bullet 7 in the previous list.
7: Well, given that keeping it working is cheap and this EntityConverter is actively being extended, even in core itself, I think it's worth the effort. Using private for this stuff is controversial AFAICT, so I tend to avoid it for now.
8: Moved them above, I'm not sure that will be enough, since we are removing the last method.

This implements:

What about adding a separate interface only the core EntityRepository implements?

Working on the IS update now.

Updated IS, I'd like to wait for the terminology discussion to settle before creating a CR draft.

Created #3035037: Entity param conversion may be slow due to available context resolution to track the performance regression.

@Berdir, #86:

\Drupal\content_moderation\ModerationInformation has several methods that seem weird/wrong and we should consider to deprecate them. [...]

Yep, once this is committed, CM and CT will need to be updated to adopt the new API, as well as other places probably.

getLatestTranslationAffectedRevisionId() can return an old non-default revision [...]

The implementation provided here is only the minimum required to make the API usable in core and let core modules play decently together. If all goes as expected, in #3023194: [PP-2] Add parallel revisioning support (or in a follow-up) we should be able to deprecate all the logic revolving around the latest and the latest translation affected revision and exploit revision parents and branching to properly track the active revision for each translation.

From the IS:

The API introduced here will only take care of negotiating the fittest variant for the specified context, access control will need to be applied on top of that.

Is there already an issue for a follow-up which addresses revision access control?

Not that I know of.

@plach and I discussed the naming again and bounced around various suggestions. Here's some of them:

- getCanonicalOf()/getWorkingCopyOf() which lead to
- getCanonicalCopyOf()/getWorkingCopyOf()

Those lead to getMultipleWorkingCopiesOf(), which makes it sounds like you are getting multiple "copies" of the same entity, and it doesn't make any sense. At that point we both thought that it's best to go back to the original names, which are very simple and easy to remember after all: getCanonical()/getCanonicalMultiple() and getActive()/getActiveMultiple().

1. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +130,194 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +    return current($this->getMultipleActiveVariants($entity_type_id, [$entity_id], $contexts)) ?: NULL;
   ...
   +    return current($this->getMultipleCanonicalVariants($entity_type_id, [$entity_id], $contexts)) ?: NULL;
   

   this will trigger a warning in older versions of php because you can't pass non-variables by reference

2. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +130,194 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +        break;
   

   nit: we could return here instead of breaking

3. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +130,194 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +    else {
   

   no need for the else here

4. +++ b/core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php
   @@ -50,13 +48,14 @@ class AdminPathConfigEntityConverter extends EntityConverter {
   +   * @param null $removed_parameter
   +   *   (deprecated) The language_manager service is deprecated in Drupal 8.7.0
   +   *   and will be removed before Drupal 9.0.0.
   ...
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, AdminContext $admin_context, $entity_repository = NULL, $removed_parameter = NULL) {
   +    parent::__construct($entity_type_manager, $entity_repository, $removed_parameter);
   

   shouldn't we be doing an instanceof check in the constructor to make sure we've got the correct service and then trigger a deprecation error if not and grab the service from the \Drupal singleton?

   we do that below in EntityConverter but not here?

5. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -125,32 +134,38 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lan
   +      $entity = $this->entityRepository->getActiveVariant($entity_type_id, $value);
   

   we can return here and avoid the else

6. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -125,32 +134,38 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lan
   +      $contexts['@entity.repository:legacy_context_operation'] = new Context(new ContextDefinition('string'), 'entity_upcast');
   

   its a pity we didn't make entity_upcast a constant, follow up? (out of scope here)

7. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -161,39 +176,25 @@ public function convert($value, $definition, $name, array $defaults) {
   +    $context_id_prefix = '@language.current_language_context:';
   

   should we make constants for this? would be on the experimental interface for the 'may change' signal

8. +++ b/core/modules/views_ui/src/ParamConverter/ViewUIConverter.php
   @@ -49,12 +47,13 @@ class ViewUIConverter extends AdminPathConfigEntityConverter implements ParamCon
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, SharedTempStoreFactory $temp_store_factory, ConfigFactoryInterface $config_factory = NULL, AdminContext $admin_context = NULL, $entity_repository = NULL, $removed_parameter = NULL) {
   

   same here, would have expected some instanceof checks and some trigger_error ?

9. +++ b/core/tests/Drupal/Tests/Traits/Core/CurrentUserTrait.php
   @@ -0,0 +1,69 @@
   +trait CurrentUserTrait {
   

   this looks great and in my opinion should be split off into its own issue as a) it is useful on its own and b) that would reduce the patch size here

@larowlan, #97:

1: According to the docs, current() doesn't expect a reference: PHP 5.0.5 does throw a notice, but I think it's just a bug.
2: 👍
3: 👍
4, 8: I'm not sure I follow: by passing those parameters to the parent constructor we do trigger notices and instantiate services via \Drupal.
5: 👍
6: Actually we can do even better, we can get rid of that entirely in #3031124: Deprecate the "entity_upcast" operation specified in EntityConverter, since apparently no one is using it ;)
7: Good point, however I think it's the responsibility of the exposing service to define those constants, so in the case of those two it would be CurrentLanguageContext. This patch adds a constant for @entity.repository:legacy_context_operation.
9: Yup, this is now postponed on #3035825: Make it easier to set up the current_user service in kernel tests. See you there ;)

Attached you can find a complete patch, a review patch, and two separate interdiffs as I messed up my local branches, sorry :)

And this reverts the method names to the original form, as mentioned in #96.

Rerolled on top of #3035825-12: Make it easier to set up the current_user service in kernel tests. No review patch as it is the same of #99.

Another reroll

I looked at the 'review' patch from #99 and i could only find two minor nits:

1. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -112,4 +130,188 @@ public function getTranslationFromContext(EntityInterface $entity, $langcode = N
   +  protected function getAvailableContexts() {
   +    return $this->contextRepository->getAvailableContexts();
   +  }
   

   This was already mentioned in #85.4 and IMO we should remove it.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityRepositoryTest.php
   @@ -0,0 +1,326 @@
   +    $this->setUpCurrentUser();
   

   Shouldn't we move this above all the entity schema installs, like we do in every other test which needs it?

In my opinion “canonical” is fine because that’s an understood term to mean what appears when the user is viewing the Entity, but “active” seems a little too ambiguous to me.

I’m concerned “active” could be confused to mean “I am viewing the Entity therefor I am viewing an active revision” and “I am editing the Entity therefor i am working with an active revision”, see what I mean?

Essentially what I’m trying to say is that developers and administrators could misinterpreted “active” and use the “active” revision when they meant to use the canonical revision, because it is “active” therefor it is “live”, “currently available for use”, “therefor it should be used”, etc.

This addresses #104.

CR draft added at https://www.drupal.org/node/3036722.

@AaronMcHale, #105:

It seems the naming issue is not settled yet: given that the API is marked as experimental, to allow unblocking the many issues that depend on this, I'd suggest to commit this as-is, given the current solution is something most people that provided feedback can live with. We can iterate over the method names in a follow-up, we could even create a poll about those.

Fully agreed with #107 :)

I think we're finally ready to go here! This issue still depends on #3035825: Make it easier to set up the current_user service in kernel tests, but setting to RTBC for extra visibility.

Just here quickly to say 'canonical' seems good, will try to do another review early next week.

The blocker was committed, one more reroll.

Regarding naming: in that case I'm fine with #107, should we make a follow-up issue to discuss naming further then?

Yes :)

Opened an issue for using the active variant in entity forms by default: #3037144: Ensure that entity edit forms use the active variant of an entity

Found a few CS problems while working on other followups.

I just realized that we should probably deprecate all the methods introduced in #2926483: Add API methods for determining whether an entity object is the latest (translation-affecting) revision and provide a single isActive() replacement for them. This is the method that, for example, Quick Edit should use instead of $entity->isLatestRevision() in quickedit_entity_view_alter(), which would ensure that Quick Edit works well with Workspaces by default.

This could be a followup, but we're running short on time until 8.7.0-alpha1 so I'd like to propose this interdiff. Looking forward to any opinions :)

Ok, here it is: #3037247: Add an API for determining if an entity object is the active variant

> retrieving the active entity variant matching the specified context: this is the variant that is suitable for editing and previewing.

Not sure if "previewing" is used anywhere in the code or so, but it clashes a bit with the node preview feature, which is completely unrelated to this issue, this is more about the "latest version" thing.

+1 on the new method names, no need for any suffixes.

1. +++ b/core/lib/Drupal/Core/Experimental/Entity/EntityRepositoryInterface.php
   @@ -0,0 +1,114 @@
   +
   +namespace Drupal\Core\Experimental\Entity;
   +
   +use Drupal\Core\Entity\EntityRepositoryInterface as CoreEntityRepositoryInterface;
   

   This introduces a new concept (the experimental namespace), a bit worried this will result in discussions on naming and so on. Not sure I like "Experimental" :)

   I know this was done based on my feedback of having @internal on a non-internal interface, but unsure about this approach. Having two interfaces with the same name might confuse people relying on autocomplete and so on (this being internal will help, but still) and it also means everyone who uses it will see a hard break in case this is then merged back into the main one.

   I think having input from @alexpott on this would be useful. Other maintainers of course too :)

2. +++ b/core/lib/Drupal/Core/ParamConverter/AdminPathConfigEntityConverter.php
   @@ -50,13 +48,14 @@ class AdminPathConfigEntityConverter extends EntityConverter {
       * @param \Drupal\Core\Entity\EntityRepositoryInterface $entity_repository
       *   The entity repository.
   +   * @param null $removed_parameter
   +   *   (deprecated) The language_manager service dependency is deprecated in
   +   *   Drupal 8.7.0 and will be removed before Drupal 9.0.0.
       */
   -  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, AdminContext $admin_context, LanguageManagerInterface $language_manager = NULL, EntityRepositoryInterface $entity_repository = NULL) {
   -    parent::__construct($entity_type_manager, $language_manager, $entity_repository);
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, AdminContext $admin_context, $entity_repository = NULL, $removed_parameter = NULL) {
   +    parent::__construct($entity_type_manager, $entity_repository, $removed_parameter);
    
   

   I still think that $removed_parameter is not necessary for BC since it is the last argument anyway. I can try to work on an alternative patch but likely not before tonight and I don't want to hold this up. We have to touch it anyway in the 9.x branch to add the type hint for $entity_repository, so its not a big deal.

Why is adding an Experimental namespace more desirable than simply providing BC according to our continuous deprecation policy?

FWIW I agree with @Berdir that using @internal on code that's intended to be extended is bad practice and just contributes to no one paying attention to @internal.

Also I'm -1 on allowing interface method signatures to change (something @plach mentioned in Slack). We tried this concept with experimental modules themselves -- people were told the whole module was not to be extended -- and then BC breaks in said modules resulted in significant disruption for the ecosystem anyway and led to perceptions that Drupal 8 is unstable. Essentially this is just a developer-centric recycling of that same concept. Unless the code is only alpha and therefore not shipped in any release, we should expect to provide BC for it.

NW for the above. Let's just do our best to get the interface and methods right here, and if we need to change them in the future let's follow the deprecation process to the best of our ability.

This addresses #122 and indirectly #119.1.

@Berdir, #119:

I updated the CR to try and follow more closely the PHP docs but sill provide a high-level summary: https://www.drupal.org/node/3036722/revisions/view/11325181/11325221.

2: Well, I think it's good to use the injected entity repository rather than retrieving it directly from the container, if people bother passing it, even it's in the deprecated position :)

@xjm:

The reason why I proposed to use that Experimental namespace was twofold:

• I agree that having @internal methods on interfaces is suboptimal and something to be avoided as much as possible, so I think it would be great to have a way to mark an API as "not mature enough yet", rather than using @internal, which additionally conveys the wrong idea, because early adopters that know what they are doing would be welcome to use such APIs, whereas proper @internal stuff is "hands-off" (and as such it would be great if it didn't live on a non-internal interface).
• Naming is hard™ and my main (only?) concern with continuous deprecation is that if you pick the right name for something you need to deprecate later on, you are then forced to pick alternative less good names, or possibly mess with namespaces or aliasing, which is a serious problem for DX and maintainability. BC is not free by any means.

We do have experimental modules and IMO we should have experimental APIs. This was probably not the right issue to introduce this idea, because one if its main drivers is to make Layout Builder stable and as such we cannot remove the API from 8.7, but I do think that a developer-centric recycling of [the experimental module] concept is something worth having or at least discussing.

The interdiff looks good to me.

I'd also like to note that #3037144-7: Ensure that entity edit forms use the active variant of an entity converts all entity forms to use the new API added here, and it seems to work very well.

Can we please add a paragraph in CN about what does it mean, which version will be loaded, if we use old methods like $storage->load() and $sotrage->loadRevision()?

I think we need a follow-up issue for views to provide the field plugins for getCanonical and getActive?

From the CR :

This new entity variant negotiation API relies on the Context API to provide information about the context in which the entity will be exposed for interaction or preview.

Reading only the CR it does not become clear how could custom contexts be defined and used to influence the decision. I think it would be really helpful to add an example for that.

This hopefully addresses #126 and #128: I added a paragraph about the storage layer, more info about contextual information, and another example using custom contexts.

Sorry if the examples are a bit vague, but unfortunately this API is rather new for myself as well, so how contextual information could be used besides what we have in mind in #3023194: [PP-2] Add parallel revisioning support is not completely clear.

I'm not sure I understand #127: Views targets specific revisions and has already extensive support for translation handling, including translation negotiation, so I'm not sure anything is needed in the CR. Of course we may want to be able to list the active variant for each entity, however this seems like a new feature to me, rather than something requiring an update. I might be wrong of course :)

I added a paragraph about the storage layer, more info about contextual information, and another example using custom contexts.

Sorry if the examples are a bit vague, but unfortunately this API is rather new for myself as well, so how contextual information could be used besides what we have in mind in #3023194: [PP-3] Add parallel revisioning support is not completely clear.

Does this mean that the new API does not make it possible to use custom contexts to influence the decision?

And then how can I influence the decision which entity revision will be loaded based on my new context?

@hchonov:

In the current (minimal) implementation only the language context is considered, however in #3023194: [PP-2] Add parallel revisioning support we are planning to allow to register revision type plugins that would be tied to one or more contexts, including custom ones.

So here we introduce a new API, which cannot be fully used and there are plans to introduce a mechanism for fully using it later?

If there is already a plan on how the API will be modified to allow custom contexts, then why isn't that part included in here and later used by the referenced issue?

The new API is confusing without this ability, at least for me, because we have a new API and we can only pass the langcode and nothing else as a context.

If there is already a plan on how the API will be modified to allow custom contexts, then why isn't that part included in here and later used by the referenced issue?

Because there is no logic that can leverage additional contexts in core at the moment. The rationale behind this choice was that this way we can unblock several issues that are currently blocked on this API not existing. Those can work well enough with what core provides (translation support + pending revisions), without us risking to introduce a full-fledged solution that has lots of moving parts and dependencies and that will never be ready in time to be shipped with 8.7. One of the main drivers for this issue is allowing Layout Builder to be marked stable and that requires at least a "stub" implementation of this API.

The API was originally meant to be marked "experimental" to reduce the potential confusion deriving from the minimal implementation, however this is no longer on the table (see #120 - #122). OTOH an alternative implementation of the entity repository could be provided in contrib and that could leverage contexts as it sees fit (https://github.com/amateescu/revision_tree is doing exactly that), so I'm not sure how to alleviate this concern besides describing in the CR how things are currently working.

This conflicted with one of my entity manager deprecation issues on EntityManagerTest, sorry :) Rerolled that.

Also, the constructor stuff is itching me, so here's a proposal for making that a bit simpler based on how we did that in recent issues. The entity_repository argument is new in 8.7, nobody outside of core is implementing that yet, so there is no reason to care about that. There is no need to babysit different historic versions of deprecated arguments as long as it is not broken. Either it's injected properly, then we use it, or it is not. Unit tests are broken by it anyway and in the end, those are the only thing that really care about it being injected.

We could even remove the "if ($entity_type_manager instanceof EntityManagerInterface) {" block based on the latest agreement because that will eventually get the trigger_error() due to still being called. But that's out of scope.

I'm only aware of a single override of EntityConverter::__construct() in contrib, that's in search_api and that doesn't even provide $language_manager yet, which is also fairly new (8.5?). This is 20 lines of code less, and the worst case is that a class that already injected language_manager gets a *slightly* less helpful deprecation message, that will be obvious when he looks at the new constructor.

And now with the correct variable name for the deprecation message.

As of #124:
> I agree that having @internal methods on interfaces is suboptimal and something to be avoided as much as possible, so I think it would be great to have a way to mark an API as "not mature enough yet", rather than using @internal

My gut feeling is that i'd prefer @deprecated for this. It's "the stuff that works now and i'll have to touch later". In this case "may or may not have to touch"...

RE: #129

I'm not sure I understand #127: Views targets specific revisions and has already extensive support for translation handling, including translation negotiation, so I'm not sure anything is needed in the CR. Of course we may want to be able to list the active variant for each entity, however this seems like a new feature to me, rather than something requiring an update. I might be wrong of course :)

hehe, I was not talking about CR at all in #127. I was just saying, did we need dedicated views field plugins for getCanonical and getActive? Now that these concepts/function exist how can I make sure my node listing view is showing the correct version of node.

@Berdir, #137:

The entity_repository argument is new in 8.7 [...]

I didn't realize this, that's why I was insisting on supporting three parameters. The change makes sense to me, however deprecation tests need to be updated as well.

@jibran, #144:

I'm not sure about field plugins, I'd have to look at the Views codebase, but I think it would make more sense to open a follow-up to discuss that in detail.

The interdiff looks good, hopefully the RTBC sticks this time :)