Field save method called twice when a page is saved

ovenum commented 1 year ago

Description

The save method from a fields config is called twice when a page is saved. Not sure if this is intended behaviour, but for custom fields that run tasks when stored (creating new pages, sending emails, …) this can have unintended side effects.

This happens on both the new v4s and v3.9.6.1. Did not test on older versions.

Expected behavior
The fields save method to be called only once. The value it returns for storage in the pages content file should be known from the first call.

To reproduce

Install the kirby starter kit. If using x-debug, add a break point at the beginning of the tags field save method, or add error_log($value) before the return statement and make sure php error logging is enabled.

https://github.com/getkirby/kirby/blob/ee2669e6dd091399fd7981381e1bd8cbd94d90a6/config/fields/tags.php#L82

Open the panel, navigate to pages/photography+trees, make a change and save it. The breakpoint should stop the program execution twice or the error_log writes twice to the log file.

Screenshots

Callstacks for the first and second time the breakpoint is hit.

Your setup

Kirby Version
v4.alpha-7 v3.9.6.1

Additional context
Not sure if this is actually a bug. Also the save method is probably not the right place to run this extra tasks. What i would like/need is a way to run tasks when a field is stored with access to the fields value before the save operation, the fields value to be stored, and the model the field is stored with.

rasteiner commented 1 year ago

The two screenshots of the callstack are identical, like... even the cropping. You sure you got the right ones?

ovenum commented 1 year ago

Uploaded a montage of the two callstacks, highlighting the call to Form::strings from the second call. That’s where they differ. Should have made it clearer in the issue description

distantnative commented 1 year ago

I think the origin is that currently $field->save() isn't much used as an action but rather to retrieve

whether the field is savable
what value should be saved

ovenum commented 1 year ago

Happy to create a feature request on kirby.nolt.io for a field save hook or a field method that is called by kirby only when a field is actually saved.

This is probably more of an enhancement than an issue, but going forward

the he documentation for field config files could explain what the save method is used for or rename the save method to something more in line with what it actually does
create two separate methods for value retrieval and whether the field is saveable

getkirby / kirby