Allow section storage to provide a more granular ID for tempstore

This seems like a good ID and could be used in

1. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
   @@ -84,4 +84,23 @@ protected function getTempstore(SectionStorageInterface $section_storage) {
   +   *   A unique string representing the section storage. This should include as
   +   *   much identifying information as possible about this particular storage,
   +   *   including information like the current language.
   

   If we want as much of information as possible should just include now as much information as we have. So add view_mode now even though it won't vary by that now.

   That way if we add that in the future an in process tempstore would still work.

2. +++ b/core/modules/layout_builder/src/TempStoreIdentifierInterface.php
   @@ -0,0 +1,15 @@
   + * @todo.
   

   Do. Do we add @internal but note that it could be put in another namespace. I could see this being useful in other situations.

I reviewed that the Id => Key in the interdiff but it probably makes sense for @tedbow to have a look. Is there any way to manually test this?

I'm still seeing the bug from https://www.drupal.org/project/drupal/issues/3028821#comment-12945393 with patch from #4.

Ok, I created but didn't save layout changes in English. When going to French URL as same user then it shows the changes that were made in English (without saving). If I added to French layout and not save and then reload the English URL as same user then I see the French changes.

Back to "Needs work".

Issue summary looks good.

the problems documented in #9 will be addressed in #2946333: Allow synced Layout override Translations: translating labels and inline blocks which this patch helps

I think this needs an upgrade path of some sort; when I applied the patch to my site and cleared caches with an existing LB tempstore I got some really weird results (a persistent message about the layout being locked that would not go away no matter how many times I saved the layout as the owner or tried to break the lock as the other user).

(FWIW I was testing the patch together with #3025231: Concurrent editing of layouts is very confusing so that the tempstore actually worked in the first place.)

No, I don't think it's OK on its own either... it's still causing the tempstore data to be effectively lost. STR:

1. Install 8.7.x HEAD and enable Layout Builder.
2. Go to admin/structure/types/manage/article/display. Check "Use Layout Builder" and "Allow each content item to have its layout customized." Save.
3. Create an article node and save it.
4. Click on the Layout tab for the article node. Add a new two-column section at the top and place a block in it. Do not click save. Note that you can leave the page and come back, or even log out of the site, and your unsaved changes with the two-column section will still be there.
5. Apply this patch and clear your cache. Go back to the node 1 layout tab. Your unsaved section is lost.

A tempstore is not a cache; it is actual stored data from user input that is supposed to persist (for a week in this case) and cannot be regenerated. So I still think it needs either an upgrade path, or some sort of fallback to allow it to find the old tempstores. Furthermore, even though #3026698: Allow section storage to provide a more granular ID for tempstore is not committed yet, the persistent "break lock" message that's impossible to get rid of does demonstrate that the data integrity problem, which may also be surfaced in other ways we haven't thought of as well, including with contrib or custom code that interacts with this tempstore.

@tim.plunkett contended that we don't need an upgrade path for data in key_value_expire that only persists for 7 days. However, I think we do need to do something for this issue, because it still is a data integrity problem for those 7 days. I'll ask other committers what they think.

Should this be manually tested again?

Walked through the patch and discussed with @tim.plunkett. I think everything here makes perfect sense. RTBC!

Inline comment is helpful and the test makes sense to me. RTBC when Christmas arrives.

So the update path fixes the data integrity problem. My data is safely there after I run update.php.

However, the "You have unsaved changes" message disappears from in-progress overrides.

For more fun, when this is combined with #3025231: Concurrent editing of layouts is very confusing, there are no warnings about unsaved changes nor any "break this lock" messages. So essentially, when someone updates to 8.7.0 with both these changes, the fix for #3025231: Concurrent editing of layouts is very confusing takes seven days to take effect. 😂

@tim.plunkett is looking into this. Setting NR for discussion.

1. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
   @@ -84,4 +84,23 @@ protected function getTempstore(SectionStorageInterface $section_storage) {
   +  protected function getKey(SectionStorageInterface $section_storage) {
   +    if ($section_storage instanceof TempStoreIdentifierInterface) {
   +      return $section_storage->getTempstoreKey();
   +    }
   +
   +    return $section_storage->getStorageId();
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -16,7 +17,7 @@
   -abstract class SectionStorageBase extends ContextAwarePluginBase implements SectionStorageInterface {
   +abstract class SectionStorageBase extends ContextAwarePluginBase implements SectionStorageInterface, TempStoreIdentifierInterface {
   
   @@ -108,4 +109,11 @@ public function getContextsDuringPreview() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getTempstoreKey() {
   +    return $this->getStorageId();
   +  }
   

   Good BC all around here.

2. +++ b/core/modules/layout_builder/src/TempStoreIdentifierInterface.php
   @@ -0,0 +1,22 @@
   + * @internal
   + */
   

   Another "why am I @internal?" here.

This looks good. I manually tested it and it works good now.

Committed and pushed to 8.7.x. Thanks!

Test introduced here hit something on PHP 7.3.x:
https://www.drupal.org/pift-ci-job/1201988
https://www.drupal.org/pift-ci-job/1202908

Drupal\Tests\layout_builder\Functional\Update\TempstoreKeyUpdatePathTest::testRunUpdates
Exception: Notice: unserialize(): Error at offset 7543 of 10758 bytes
Drupal\Component\Serialization\PhpSerialize::decode()() (Line: 21)


/var/www/html/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:51
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:203
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:156
/var/www/html/vendor/guzzlehttp/promises/src/TaskQueue.php:47
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:246
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:223
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:267
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:225
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:62
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:131
/var/www/html/vendor/fabpot/goutte/Goutte/Client.php:180
/var/www/html/vendor/symfony/browser-kit/Client.php:312
/var/www/html/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:144
/var/www/html/vendor/behat/mink/src/Session.php:148
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:326
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:333
/var/www/html/core/tests/Drupal/Tests/UiHelperTrait.php:532
/var/www/html/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:297
/var/www/html/core/modules/layout_builder/tests/src/Functional/Update/TempstoreKeyUpdatePathTest.php:34