Layout builder should use the active variant of an entity to avoid orphaned revisions

See also #2937199: Track Layout override revisions on entities which support revisioning

My thinking here is that LB should work with CM, in so far as a new layout should be a new pending revision.

Data loss makes this critical

Should be job of LB?

Most of \Drupal\Core\ParamConverter\EntityConverter has nothing to do with param converters, it's just "the right way to load entities for editing/displaying".
If that was abstracted to a service, I think the right method calls added to \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::getSectionListFromId() would solve this.

This REALLY needs functional tests to validate that it's the behavior we want.
But this might be a start, as I understand the problem?

Oops, had other code applied locally.

Anyway, I think this should NOT be Layout Builder's responsibility at all. There should be an API method on a service for loading a mutable entity, and we should call that instead of $this->entityTypeManager->getStorage($entity_type_id)->load($entity_id); which should be banned from usage in code that presents user-facing data.

Tim brought up the comparison of get() vs getEditable() for config objects, and that really resonated as a good analogy to me on this topic. I think it's fine that layout_builder acknowledges that it needs to make concessions to some other system in order to get the particular version of the entity it needs, but that could be revision, or revision of a particular language, or any number of unforeseen criteria that may come to light at some point in the future. As it is, this patch hard-codes the logic which should probably exist in a real api somewhere. If we had a central service that could negotiate that, I bet stuff like content_moderation would get a lot easier and we'd not have to play whack-a-mole with all the new modules that come along that we wish we could integrate with.

Eclipse

Updated the IS to clarify that we are not talking about data loss, but pending revision data being overridden by new default revision data.

In that light I'm not sure this should be critical, given that Layout Builder is still experimental. Not changing the priority, since this will for sure be discussed during the next triage call.

Btw, what #17 is suggesting looks very similar to what Sam is suggesting in the OP. Unfortunately, even if we had that, it still one more concept to deal with, but I'm afraid there's no way around that.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -126,7 +128,20 @@ public function extractIdFromRoute($value, $definition, $name, array $defaults)
+        $revision_id = $entity_storage->getLatestRevisionId($entity->id());

Btw, this is not even correct from the multilingual perspective. So one more reason to add an API to encapsulate this logic.

Here's what I think Layout Builder needs. The rest is up to Drupal\Core\Entity to solve.

I am downgrading and blocking this on #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing.

Let's start using the "active revision" terminology :)

Tagging as this is currently on our shortlist.

No one is actively working on this.

I think this is still critical, because even if #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing is addressed the data integrity issue is still there until LB and/or CM implement it.

Will read the above issue.

Is there a new temporary shim to help with this problem? The temporary shim being to have layout builder always load the latest revision.

I was using the patch in #17 (https://www.drupal.org/files/issues/2942675-layout_revision-15.patch), but it no longer applies to the latest version of core, currently 8.6.7. Also, manually applying its updates didn't make any difference since the method affected is deprecated and layout builder is apparently no longer using these deprecated methods.

The patch in #22 (https://www.drupal.org/files/issues/2942675-layout_entity_thing-22.patch) doesn't apply either. It looks like it prepares for a solution, but on its own, doesn't provide a workaround.

Here's a reroll of #17 for 8.6 and 8.7
I would also recommend following #2946333: Allow synced Layout override Translations: translating labels and inline blocks

Thank you for the reroll Tim! I've successfully applied it on top of the patch in #2986394: 2.x patch reroll issue (I'm using lightning which includes lightning layout), and it seems to be working as expected now. I realize this is kind of a superficial fix to a really tricky problem, so I'll try to follow along with the other related issues.

typo

Here's a more heavy handed approach building off of #2942907-84: Entity system does not provide an API for retrieving an entity variant that is safe for editing.
Going to use this patch alongside other issues to help prove their test coverage.

Accidentally included the test from #2973860: When there is an entity forward revision for Content Moderation and a layout override is saved the revision is no longer latest here. Removing that.
Also changing the prophecy until the new methods are on the interface.

I'm not sure how hard to push on this.
Mostly it's an easier means for other issues to be tested than having to apply both the most recent version of that patch plus the internal changes, but updated for the method names du jour.

The internals of the other issue haven't changed in a bit.

This is also an escape hatch if the other issue stalls out and we need something to help fix all these stable blockers.

#2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing landed!
Let's repurpose this for using the new API?
Or close it as a dupe and implement this in an issue with tests (like #2973860: When there is an entity forward revision for Content Moderation and a layout override is saved the revision is no longer latest)?

We could commit as-is, but I'd definitely prefer we get the test coverage from the issues that had been blocked by this API with us.

Patch looks great.

The patch looks good to me, I manually tested it and it indeed fixes the reported issue. However test coverage looks a bit shallow: what about adding a functional test simply checking that the latest revision is loaded on the /node/{node}/layout route when we are dealing with a pending revision? We could create a pending revision manually to avoid having to set up Content Moderation.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -74,15 +75,23 @@ class OverridesSectionStorage extends SectionStorageBase implements ContainerFac
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, SectionStorageManagerInterface $section_storage_manager, EntityRepositoryInterface $entity_repository) {

No deprecation/BC layer here: I guess that's fine given that we are in an experimental @internal plugin.

Good idea, I feel much better having a functional test here.

FAIL patch is equivalent to the interdiff

+++ b/core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php
@@ -119,20 +127,17 @@ public function providerTestExtractIdFromRoute() {
+    $this->entityRepository->getTranslationFromContext(Argument::cetera())->shouldNotBeCalled();

Why wouldn't getTranslationFromContext() be called? Maybe this line should have a comment explaining the train of thought here.

Otherwise, every line in this patch makes crystal clear sense to me.

That check for no calls to getTranslationFromContext was helpful when debugging this, but keeping it in here now is coupling this too closely to knowledge of the internals of EntityRepository. Removed

Thanks, @tim.plunkett! RTBC once Christmas-ey.

Nice spot! That indeed looked suspicious :)

Saving credits

Committed 17de81c and pushed to 8.7.x. Thanks!