getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Panel doesn't update from php values after save #3972

Closed Daandelange closed 2 years ago

Daandelange commented 2 years ago

Describe the bug
When the panel saves the changes of a page field, the Kirby API sends back the updated values (observed in devtools network monitor). My bugreport, or question (or feature request?) is if the panel is supposed to update using the $store's model.changes (as currently), or rather update using the received PHP values.
If PHP is the answer, I found a bug : the values ($store.model[content].originals) are not updated with the newly received values, but with the changed values from the $store.

https://github.com/getkirby/kirby/blob/9ea05e38dbeb471657e3066651568fc29763ef06/panel/src/store/modules/content.js#L308-L318

I've had success changing the above lines to this, and it's very useful to me, as a php change to the field values is then reflected to the panel :

let response = await Vue.$api.patch(model.api, data) // Note: data is const, cannot be mutated, and patch() doesn't either try to modify it.

// re-create model with updated values as originals
context.commit("CREATE", [id, {
...model,
originals: (response && response.content) ? response.content : data // <-- prefer updating from php values, otherwise use latest available js values
}]);

After Bastian's 3.6 presentation video (last week), I understand that the intention of Fiber is to reflect php changes 'transparently' to javascript, preventing the need to write js where possible; so I thought this might be a bug (or feature request?). If not, sorry for filling an issue here.

For the details, my particular use case is to modify/sanitize a custom field's values by implementing a custom $field->fill($value);. (is this the correct place to do this?) While parsing values from content file, it works perfectly, and updating from the panel too, but the changes only visually apply when I reload the panel view.

To Reproduce
Not really needed to reproduce, reading the code above should be enough to understand. I think that all Kirby core fields have identical js and php values at any time, so to see the bug (the difference), one needs a custom FieldClass that modifies its value on save/load. (custom fill() implementation). If needed, I can share some code.

Expected behavior (or wished behaviour?)
The panel uses networked/php values to update the content model instead of the cached ones. [If this is not the expected behaviour, I'd expect the API not to send back the new values ?]

Kirby Version
Kirby 3.6

Daandelange commented 2 years ago

I'm making quite a few custom fields that change/sanitize the value on save on the PHP side. (on fill() or save() depending on if the field is extended trough PHP classes or blueprints respectively)

I'd love to know if this is considered a bug or if this is the expected behaviour (that the PHP changes are not reflected on the Vuejs side until the page is reloaded).

lukasbestle commented 2 years ago

There is one edge case with replacing the content with the data from the backend: It means that any changes that were made while saving are lost, see https://github.com/getkirby/kirby/issues/2554.

Apart from that, I'd say that the expected behavior should certainly be to update the frontend with the content from the backend. So I consider it a bug if it doesn't happen.

@Daandelange While the links to the relevant code are really helpful, could you please still post a simple reproduction example that allows us to verify that the resulting fix actually solves the problem?

Unfortunately I cannot promise a quick solution at the moment. The Panel form setup is in need of a refactoring as it has all sorts of issues that cannot easily be solved individually. We therefore have an internal list of all form-related issues so we can solve them all during the refactoring. In this list we track this issue as well. The current refactoring project was the date fields and we are in the process of prioritizing future refactoring projects.

Daandelange commented 2 years ago

Thanks for your reply, here's some code to reproduce it :


Daandelange commented 2 years ago

As a side note, I've been using my Kirby localhost/dev installation with the content.js fix above since I created this issue.
Without extensive testing, I haven't run into any particular issue; having tested several field-types with it without noticing any side-effect other than my custom fields correctly working this way (without page reload). Indeed, the issue you mentioned could still be a side effect.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.