"Unsaved changes" message incorrectly appears on layout builder

Couldn't progress much. Hence unassigned myself

Can confirm this is still an issue in 9.3.

Needs automated tests, but manually testing the patch solves the problem.

I'm sorry to say that I was a major part of that issue that seems to have caused this regression. :(

The least I could do is offer a fail test patch here! Attached.

And there's another patch with the FAIL test combined with the fix from comment #5. This all worked locally. Fingers crossed.

Those test results are promising. I didn't need to allow overrides to reproduce the bug manually. I just had to refresh any layout builder edit form, so that's all the fail test does. If the community thinks that's a fine test, maybe someone could remove the "needs tests" flag.

I know that @bgreco offered up the patch in #5 more as a "workaround" but it seems to work. I'd say to bring this patch up to being "committable" we would need to add hasUnsavedChanges as an official documented property on SectionStorageBase. We might even want to add a related function to get its value, though that might be overkill.

@danflanagan8 is 100% correct that this is a workaround (while a very good one!)
Setting a dynamic non-declared property like that has performance implications (see https://softwareengineering.stackexchange.com/a/186446)

Additionally, not all implementations of SectionStorageInterface are guaranteed to be extending SectionStorageBase.
So, if we want to continue down the path of storing this on the section storage, a method (or several) will be needed.

Additionally, the code currently sets hasUnsavedChanges to TRUE, but never unsets/sets to FALSE. I'm guessing this works because once we save the section storage, it's no longer used during that request, so the next time it is used it is loaded from scratch and won't have that property set.
But, there could be a case now or in the future in which we DO use the section storage before reloading it, and this could cause unintended side effects there. So if we're sticking to this approach, we need to unset it/set it FALSE.

However, I am wondering if the section storage itself is the best place to do this. Might there be some approach where we improve layoutTempstoreRepository itself to track this?

And as I read the code, this currently just means that two calls to LayoutTempstoreRepository::set() mean this flag is added. And right nowthat only happens when an actual change is made (since we now ALWAYS call set() once at the beginning).
But nowhere do we check that a change was actually made... Idk if this matters, just pointing it out.

I think the tests are sufficient for now, thanks!

not all implementations of SectionStorageInterface are guaranteed to be extending SectionStorageBase

@tim.plunkett, good point. Adding functions to the SectionStorageInterface would be an API change and would cause problems for contrib and custom code that happens to use that interface. My gut says we should try to avoid that if we can. Anyway, it seems reasonable that the tempstore would be the thing keeping track of whether or not there are unsaved changes.

Update: Although I guess updating the tempstore to handle this probably involves changes to LayoutTempstoreRepositoryInterface. Maybe we can't get around API changes here.

Counter to my point about SectionStorageBase usage not being guaranteed, core's BC policy allows for *additions* to interfaces where there is a 1:1 pairing of interface and base class.

So additions to either LayoutTempstoreRepositoryInterface or SectionStorageInterface are definitely on the table still.

@tim.plunkett, that is good to know!

Regarding which class should have responsibility for keeping track of the existence of unsaved changes, I wonder if the work on rearranging sections might help guide this: #3080606: [PP-1] Reorder Layout Builder sections. I haven't dug into that issue or patches much, but moving sections seems like a case where we would want to display a "You have unsaved changes" message even though none of the sections themselves have been modified.

Clarification: The paragraph above treats SectionStorage as if it corresponds to a singleSection. As I've read more, SectionStorage references a list of Sections. So re-arranging Sections in fact does change SectionStorage. So the referenced issue won't likely provide as much direction as I had hoped!

Here's a patch where I attempted to keep everything in the tempstore. It's unrelated to the original fix, so no interdiff. It contains the fail test from comment 8.

1. +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
   @@ -66,10 +66,10 @@ public function onPrepareLayout(PrepareLayoutEvent $event) {
   -    if ($this->layoutTempstoreRepository->has($section_storage)) {
   +    if ($this->layoutTempstoreRepository->hasUnsavedChanges($section_storage)) {
          $this->messenger->addWarning($this->t('You have unsaved changes.'));
        }
   -    else {
   +    if (!$this->layoutTempstoreRepository->has($section_storage)) {
   

   I LOVE this change, it is so clear (in a piece of code that has never been clear, in it's multiple iterations). Huge +1 to this new method

2. +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
   @@ -79,7 +79,7 @@ public function onPrepareLayout(PrepareLayoutEvent $event) {
   -      $this->layoutTempstoreRepository->set($section_storage);
   +      $this->layoutTempstoreRepository->add($section_storage);
   
   +++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
   @@ -52,12 +52,40 @@ public function has(SectionStorageInterface $section_storage) {
      public function set(SectionStorageInterface $section_storage) {
   ...
   +  public function add(SectionStorageInterface $section_storage) {
   

   The difference between set and add is unclear, however. If we need to have two different methods, we should be clear about which one should be called when, and what the difference is.

   Another option would be to add an optional second parameter to set(), like $check_unsaved_changes = TRUE or something. This would mimic the patch in that all other calls to set() would get the new behavior, but where we call add() above could be ->set($section_storage, FALSE);

Another option would be to add an optional second parameter to set()

I'm fine with that.

I did however discover another problem with the "Unsaved changes" message (I think) that isn't fixed in any way by the work here. I went back to the issue that caused this regression and went through the test steps outlined in the IS.

#3144010: New pseudo-fields cannot be removed, InvalidArgumentException thrown

After Step 6 we see the new extra field block on the form. At this point the extra field block is not yet saved to the layout. Therefore, we should expect the "Unsaved changes" message. That message does not appear though.

That's kind of a weird scenario though, because if one were to view an article after Step 6 but before Step 7, one would see the new extra field. But it's not in the layout configuration, though, as far as I can tell. If I export the Entity View Display config after Step 6 but before Step 7, I don't see the new extra field anywhere in the configuration.

@tim.plunkett What do you think about this scenario with the new extra field? Am I right that's another bug with the unsaved changes message? If so, should that be tackled within this issue?

I'm uploading a new patch with the broadened fail test.

if one were to view an article after Step 6 but before Step 7, one would see the new extra field

So the output is "correct" (as expected) without having saved the layout? I think this is just due to the way that extra fields are rendered...
In that case, I don't know that anything is wrong here. If the save isn't "needed", then neither is the message IMO.

So the output is "correct" (as expected) without having saved the layout?

I suppose you're right. The output matches the Layout Builder UI even if the configuration doesn't match what's shown in the Layout Builder UI.

I think this is just due to the way that extra fields are rendered...

I checked the behavior with the normal Field UI and it's essentially the same thing as what we're seeing with LB. If I install a module that adds a new extra field, the extra field shows up in the Field UI and it shows up when the entity is rendered. At the same time, the new extra field isn't in the Entity View Display configuration anywhere. I'll just file this under "extra fields are weird".

So I'm happy to retract my bug report! I've canceled the test on the overzealous patch on #17.

This patch ditches the new add function in favor of adding a parameter to the set function.

Verified and tested patch #20.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Seven Theme
# Enable layout builder for a content type.
# Click the "Manage layout" link.
# Refresh the page.
# Observe the results.

Expected Results:
# The message ("You have unsaved changes") should not appear, since there are no unsaved changes.

Actual Results:
# Currently before applying the patch, the "You have unsaved changes" message appears.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

So, what is the proposed resolution here? The IS show some options, which one has been decided upon? Can we have an IS update to help reviewers.

Thanks.

@quietone, I've updated the IS. Let me know if you need to me to expand anything further.

@danflanagan8, thanks! That is very helpful. I have removed the tag. Setting to NR for a code review of the latest changes.

Appied path working fine after patch the Unsaved changes message has gone
for ref shairng screenshot

Thanks for the patch

Hi, changes look good.

Steps performed:
(1) Reproduced the issue.
(2) Applied patch 3207875--20.patch.
(3) Code review on changes.
(4) Tested again with different combinations. Issues is fixed.

Moving to RTBC.

Thanks!

1. The tracking of state with a flag seems like it could go wrong. It seems pretty odd that this flag can be set externally to the LayoutTempstoreRepository service. I see there's been some debate about this above and there was an add() method at one point. I don't know but have this on the set() method does seem very odd.

2. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepositoryInterface.php
   @@ -27,8 +27,10 @@ public function get(SectionStorageInterface $section_storage);
   -  public function set(SectionStorageInterface $section_storage);
   +  public function set(SectionStorageInterface $section_storage, $has_unsaved_changes = TRUE);
   

   As we're adding a new param let's type hint with bool. One less thing to fix when we eventually add typehints everywhere.

3. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepositoryInterface.php
   @@ -41,6 +43,18 @@ public function set(SectionStorageInterface $section_storage);
   +  public function hasUnsavedChanges(SectionStorageInterface $section_storage);
   

   As we're adding a new method lets add a return type hint of bool. One less thing to fix when we eventually add return typehints everywhere.

Thanks, @alexpott.

Addressing 27.2 and 27.3 will be easy.

But what about 27.1?

Would you prefer that we reimplement the add function? The only objection that @tim.plunkett had to that function (see #16) was that I didn't offer clear enough documentation for the difference between add and set.

We grabbed this patch perhaps a little overzealously and are in the process of rolling it back. It seems like the original issue might need to be re-assessed (we are trying our own version of that subscriber that does not eagerly create the override - as we do not have the original issue with psuedo-fields).

Anyway, I wanted to mention something I noticed. Just removing message might be more problematic than leaving it. I had the patch applied and was trying to revert a page and it did not seem to be working. I then 'discarded changes' (keep in mind, i had not message saying i had overridden changes) and boom .. my reverted node shone through.

Put another way, with this patch applied an author would struggle to revert a page after they have visited the layout tab, because they will necessarily know they first have to discard changes.

@AndyThornton could you please share your solution ("own version of that subscriber")?

Patch from #20 doesn't work with 10.3.0 because of "Add static caching to LayoutTempstoreRepository"

https://www.drupal.org/project/drupal/issues/3445909

Applied changes from patch #37 to the corresponding branches and created mr

MR appears to have test failures.

Hello,

I merged latest code changes from base branch into fork's branch and rerun the pipeline. Testcases are passed. Please review.

Thank You!

Left comments on MR.

Hello,

Thank you for reviewing the code. I pushed the code for comment on MR. Please review.

Thank you!

Verified the issue on 11.x-dev.

Steps to be followed to verify the issue.

Scenario 1:

1. Navigate to any content type by admin > structure > content types.
2. Edit content type by clicking manage fields.
3. Navigate to manage display tab.
4.navigate to layout option section and enable "use layout builder"
5.Save the changes and click "manage layout" button.
6.Refresh the page and you will see "unsaved changes" message.

Scenario 2:
1. Navigate to any content type by admin > structure > content types.
2. Edit content type by clicking manage fields.
3. Navigate to manage display tab.
4.navigate to layout option section and enable "use layout builder" and "Allow each content item to have its layout customized."
5.Create a node and Click the "Layout" tab.
6.Refresh the page and you will see "unsaved changes" message.

Issue was there in both scenarios before applying the MR!9333.

After The MR!9333 issue resolved for both scenarios. I suggest move this issue to RTBC

Attached screenshots for reference.

Feedback appears to be addressed

Saving credit.

I made two comment changes using the suggestions feature. Then phpstan was failing so I regenerated the baseline as well as properly wrapping one of the comments. There were no code changes.

Therefor I will leave this at RTBC.

Kudos to whoever decided to put the sections inside a top-level key named 'section_storage' so we don't have any BC update path issues here.

It looks like #27.1 has not been answered yet, back to NR for that

Patch from #37 worked for me. I was getting following error.

Fatal error: Declaration of Drupal\layout_builder\LayoutTempstoreRepository::set(Drupal\layout_builder\SectionStorageInterface $section_storage) must be compatible with Drupal\layout_builder\LayoutTempstoreRepositoryInterface::set(Drupal\layout_builder\SectionStorageInterface $section_storage, $has_unsaved_changes = true) in C:\laragon\www\commerce\web\core\modules\layout_builder\src\LayoutTempstoreRepository.php on line 80

Hi, I’ve tried patch #37 , and it worked for me. Please find the attached screenshots as evidence.
Before patch

After patch

Thank you!

Please don't put an issue back to RTBC without addressing the question/reason it was set the needs review

We need to answer the question in 27.1

Thanks

I would like to add one more thing regarding patch in #37 is that i had to apply it manually. I am using commerce kickstart V3. It has latest version of drupal 10.

On looking closely, I noticed that some part of the patch was already applied. I guess that is why it was not applying automatically.

Thought it may help.

....

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Hi, I've tried the patch from #37 , and it worked for me most of the time, but there's one case where the issue still occurs.

When I create a new page and immediately go to the Layout Builder, I see the message "You have unsaved changes" on the blank page. If I save the page, the message disappears. Any subsequent saves don't trigger this message — it only happens on a fresh, newly created page.

Patch 3207875--20.patch that worked before now does not apply for Drupal 10.4.5
I fixed it in this patch.

#64 works good.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The patch changes from #64 need to be incorperated in the MR(s) listed as part of this issue.

As suggested in comment #67, I’ve pushed the changes from the patch in comment #64 to the 10.3.x-3207875 branch.
Please let me know if I missed anything or if there’s anything else that needs to be done.

The latest changes to the 10.3.x-3207875 branch have caused errors. Please be careful with the code you modify.

Regards.

It would be good to get the 11.x MR up to date with these changes too, fixes should go there first :). Thanks!

I have updated the patch for the 11.x branch to include the recent changes and fixes. Please review and let me know if there’s anything that needs adjustment.

I followed the Scenario 1 steps to reproduce the issue and #81 is working fine.

Attached before and after screenshots.

Left a comment in the MR.