[v5] Running page save/update in after hook results in phantom unsaved changes notification

[adamkiss]

Description

If you need to change something in a page automatically - in my case auto-generate title based on other fields - it was always best to do so in a page.update:after hook. In v5-beta.1, the originating update is saved correctly, the hook is run (and change persisted), but the tracking system (sometimes) incorrectly keeps the page marked as changed (or re-marks, maybe?).

Sometimes the yellow buttons popup right away, sometimes after refresh, sometimes they don't pop up at all and page isn't marked changed.

I observe this pretty often (> 80%) when the page is saved with ⌘S, but trigger it slightly less (60%?) while saving by clicking the save button.

Expected behavior

1. Page is saved
2. Hook runs
3. Page isn't marked by system as having changes

Screenshots

CleanShot.2024-12-16.at.22.27.28.mp4

To reproduce

1. Have a bluprint marked with changeTitle: false
2. Add following hook:

[
	'page.update:after' => function (Page $newPage)  {
		kirby()->impersonate('kirby');

		$p->save(['title' => 'Automatically changed title at '.time()]);

		kirby()->impersonate(null);
	}
]

3. Update a page of that blueprint couple of times and observe mayhem.

Note: additionally, I don't remember having to release the kirby impersonate call before, but now it triggers the "other user is editing this page" popup.

Your setup

Kirby Version
v5-beta.1

[medienbaecker]

I experienced this issue for a simple "last update" field. I'm using update() instead of save() but the issue is the same:

'hooks' => [
  'page.update:after' => function (Kirby\Cms\Page $newPage, Kirby\Cms\Page $oldPage) {

    $date = date('d.m.Y H:i');
    $newPage->update([
      'last_update' => $date,
    ]);
  }
]

[distantnative]

Yes, the issue right now is that what's stored on the server is suddenly different from what's in the browser/was sent to the server.

@bastianallgeier @lukasbestle
We have spared ourselves so far from it, but was already discussed in the concepts. We will need the publish call (as this is the one that triggers the hooks as well) to send back the actual final content to the Panel. Incl. handling of any additional changes that were might in the Panel while the request was running.

[bastianallgeier]

Ok, I just tried a few things. Good news: we already make sure that the hook results get updated properly when the changes are published. It actually works really well.

CleanShot.2025-01-15.at.12.40.25.mp4

@adamkiss's hook solution is still correct. I'm not entirely sure why it fails sometimes. It could be related to interval at which our changes are stored on the server. This might indeed be a performance problem on different environments.

@medienbaecker your solution leads to an endless loop, which is ok though. When you call update within an update hook, the update hook will be triggered again and again recursively. Using save is the better bet here.

[medienbaecker]

I've changed my hook to use save() instead of update(). It makes sense, and I don't even understand how it worked in Kirby 4 without an infinite loop 🙂

Unfortunately, I can also still reproduce the issue of weird phantom changes. Sometimes I have to press save two times to actually save the changes. That's even more scary than the other way around (having saved but seeing unsaved changes). I could see that happen on a good server. Not sure if that's helpful, but I wanted to share.

[bastianallgeier]

@medienbaecker it's a combination of issues here.

The changed behavior of the hook is probably coming from a refactoring where we introduced that hooks can modify their input and return the results. I'm not entirely sure yet why the loop protection is affected by that but since it's the only change in our hook system it must be related.

We had a second issue that caused a loss of content changes during input and I think it's related. This is fixed in beta 2 and might already fix your issue here as well.

Then there's still a chance that our requests to store changes and the request to publish them somehow collide. That's something we need to look into again.

[jirichlebus]

I can confirm that I am registering this strange saving behaviour. Sometimes I need to click on the save button. And then (when the button is no longer visible) press ctrl+s to actually save the changes.

[bastianallgeier]

@medienbaecker @jirichlebus can you check two things for me please?

• is it still happening in 5.0.0-beta.3?
• is it happening on page views that don't use the writer, blocks, or layout fields or any other setup that might include the writer?

[jirichlebus]

@bastianallgeier I can say that in 5.0.0-beta.3 it works fine for me. However, it should be noted that I don't have a Writer field anywhere.

[tobimori]

I have this issue when going out of a Structure field popup and not making any changes, in beta.3 - not sure if the same or a different issue.

[jirichlebus]

I found issue when trying save changes via keyboard – CMD + S. when pressed, sometimes are changes saved, sometimes not.

[bastianallgeier]

We have a solution now that really seems to fully fix all the weird saving issues.

@medienbaecker we also know why the nested update call no longer works. We switched our hook system from trigger to apply to actually be able to return modified objects. But this no longer protects from nested calls. We discussed this and really wonder if we should add that protection back in. We believe that it should simply not be possible to nest an update within an update call. Our nested call protection in the Hook trigger blocked additional hooks. It solves the problem on the surface, but of course it might cause other unexpected problems because the hooks no longer get triggered again. That's why we think that the save method really is the better idea here.

[medienbaecker]

I totally agree. Just keep in mind the forum has quite a few mentions of this page.update → update hook so it would be good to mention this breaking change. It's easy to miss this one and it could potentially break things after people update to v5.

[bastianallgeier]

One big step ahead: With this PR #7053 you can call ::update within an update hook again.

[bastianallgeier]

Happy to report, that this is finally fully fixed 🚀