Once previewed in XB an entity with no changes will still show up in "Review x changes"

tedbow → credited jessebaker → .

I closed 🐛 Review changes panel shows changes even when nothing has changed Active as a duplicate of this. 👍

Only do auto-saves for entities that are different than their saved (live) versions.

Alternatively: compute the data_hash for the saved entity and check if it's different.

Having read \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable() (which creates the auto-save entry for the edited content entity using client-side representation) and \Drupal\experience_builder\Controller\ApiLayoutController::get() (which computes client-side representation to boot the XB UI), it AFAICT should be possible to just compute the hash, using something like:

diff --git a/src/AutoSave/AutoSaveManager.php b/src/AutoSave/AutoSaveManager.php
index 0cb1419c..322b93cb 100644
--- a/src/AutoSave/AutoSaveManager.php
+++ b/src/AutoSave/AutoSaveManager.php
@@ -5,6 +5,8 @@ namespace Drupal\experience_builder\AutoSave;
 use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\TranslatableInterface;
+use Drupal\experience_builder\Controller\ApiLayoutController;
+use Drupal\experience_builder\InternalXbFieldNameResolver;
 
 /**
  * Defines a class for storing and retrieving auto-save data.
@@ -27,6 +29,22 @@ class AutoSaveManager {
     return $this->tempStoreFactory->get('experience_builder.auto_save', expire: $expire);
   }
 
+  public function hash(EntityInterface $entity, ?array $data): string {
+    // Generate a hash for the saved entity.
+    if ($data === NULL) {
+      $field_name = InternalXbFieldNameResolver::getXbFieldName($entity);
+      $tree = $entity->get($field_name)->first();
+      $data = [
+        // @todo ::buildRegion is not currently callable, but *could* be.
+        'layout' => ApiLayoutController::buildRegion('content', $tree, $model),
+        'model' => $model,
+        // @todo ::getEntityData is not currently callable, but *could* be.
+        'entity_form_fields' => ApiLayoutController::getEntityData(),
+      ];
+    }
+    return \hash('xxh64', \serialize($data));
+  }
+
   public function save(EntityInterface $entity, array $data): void {
     $key = $this->getAutoSaveKey($entity);
     $auto_save_data = [

In \Drupal\experience_builder\Controller\ApiPublishAllController::__invoke we have logic to convert auto-save data to entities ready for validating/saving. We may want to move this logic into the auto-save manager(or somewhere else) so we can then compare the entities to the saved version. If there no changes to the saved versions we would then not create the auto-save entry and if there already is one delete it.

AFAICT \Drupal\experience_builder\Controller\ApiPublishAllController::validateExpectedAutoSaves() does not look at the saved data at all currently? So I don't think that logic helps.

I think that given #3501600-23: Split 1 PageTemplate config entity into N PageRegion config entities → , this is now actually a critical.

@wim leers

re #6

AFAICT \Drupal\experience_builder\Controller\ApiPublishAllController::validateExpectedAutoSaves() does not look at the saved data at all currently? So I don't think that logic helps.

I wasn't referring `validateExpectedAutoSaves` but I different part of what is now \Drupal\experience_builder\Controller\ApiAutoSaveController::post.

I pushed up a MR but I am not sure this idea would actually work. I added comments to the MR as to why

but here is the basic idea would be to change \Drupal\experience_builder\AutoSave\AutoSaveManager::save to something like
<?
public function save(EntityInterface $entity, array $data): bool {
$key = $this->getAutoSaveKey($entity);
$entity_from_autosave = $this->updateEntityFromAutoSave($entity, $data)

if ($this->entitiesEqual($entity, entity_from_autosave)) {
// The client data NOW matches the saved entity, delete auto-save if exists
if ($this->getTempStore()->get($key)) {
$this->getTempStore()->delete($key);
$this->cacheTagsInvalidator->invalidateTags([self::CACHE_TAG]);
}
return FALSE;
}
// Do current logic here...
return true;

@wim leers re #6
and your public function hash(EntityInterface $entity, ?array $data): string { idea.

This might work better than my MR but it should really have to handle all of our entity types we support because this could happen with code components or asset libraries, either because saved one was just opened in the editor and no changes were made yet or changes have been made but the user reverted those changes and now the code component or library is back to the saved state. In that case it should not be in the "Review changes" list any more

So any call to \Drupal\experience_builder\AutoSave\AutoSaveManager::save should

1. only save if the data if it is different the save entity
2. Delete any previous auto-saved state if the data is the same as the save entity

If we want to keep this issue to the current scope of "content entity or PageRegion" as the title suggest that might be good idea but we should think how might solve this for other types in the future.

I think with the way our front-end is you are more likely to get into this situation with content entities then say an Asset Library entity. I am not sure if auto save call from the client for the Asset Library gets called just by opening the form

Leaving 3502902-doomed-idea MR open for now but I think the 3502902-create-entity-hash MR started from @wim leers idea in #6 is probably the way to go.

Interesting how this MR seems to be touching upon/would benefit from infrastructure improvements we've been delaying to keep moving forward on features — this issue seems to be forcing us to do some of that. @tedbow specifically cited #3499632 in his MR comments.

Great to see this starting come together!

I reproduce the error block-form.cy.js manually. The block form doesn't not work when a region besides the main content

I haven't go block-form.cy.js passing yet.

I think what os happening is since the regions were always autosaved even if there were no changes the code path where there wasn't an auto-save probably never was tested or worked.

Manually testing if I force the region to be auto-saved in \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable then I can edit the branding block that is default placed in the header, otherwise it doesn't work.

also if put another component in the header which also triggers the region to be auto-save then I can edit both components.

So don't think it is a problem with block forms themselves it is just we only have blocks as default component in regions.

Unassigning myself but will pick it back up tomorrow

Hopefully tests will pass now. I still need to self-review/clean-up the MR

tedbow → closed merge request !686

This simpler alternative is definitely easier to land! Especially because @larowlan sprinkled comments on the MR that walk you through what's going on 👍

Still needed:

1. The simpler MR is missing 3 important parts of @tedbow's test coverage expansion
2. @jessebaker feels quite confident we can remove the 11 second timeout — let's try that.

What a rollercoaster this issue has been 😅 @tedbow did one MR, then another. Then @larowlan did a third, and explained why.

Then today, @tedbow and I iterated on @larowlan's third. Seems like @tedbow and I are mostly happy with where things are at, and @tedbow's key refactor was approved by @larowlan, and my subsequent iteration on that was approved by @tedbow, and by @larowlan. He did raise a performance concern, but I think I was able to address that adequately.

@larowlan felt that the test coverage from @tedbow's second MR didn't add much value — and while I see his point, I think there's indirect value for it, so we respectfully disagreed, but @larowlan did indicate he didn't consider it blocking 👍

Finally, 2 follow-ups were identified:

1. @tedbow did discover one thing that's broken, but that is broken in HEAD, too. @larowlan also agreed it could/should be a follow-up. Created: 🐛 Auto-saving of blocks needs to handle string-to-bool fixing Active .
2. @larowlan observed what neither @tedbow nor I observed: that we should be solving this also for those 2 other config entity types. Follow-up created: 📌 Only auto-save JavaScriptComponent/AssetLibrary config entities when there are actual changes Active .

Thanks, everyone! 🚀

wim leers → closed merge request !689

To make this issue's work much more impactful: 📌 Status badge and "Review changes" only update after ~10 seconds Active .