Skip to content

Fix unsetting field values in changes #7116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: v5/develop
Choose a base branch
from

Conversation

bastianallgeier
Copy link
Member

Description

When passing null as field value when saving changes, the value from the latest version would be magically merged back in. This PR fixes this regression.

Changelog

Fixes

  • The focus point can be removed again, once set. [v5] Image focus can't be removed #7022
  • Passing null as field value will remove the field correctly from the latest version, when publishing changes.
  • Version::isIdentical() is now comparing two versions more reliably, by not merging original values.

Enhancements

  • New merge: true argument for Kirby\Form\Form::for() which will disable merging original values. Merging is activated by default.

Breaking changes

None

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add changes & docs to release notes draft in Notion

@bastianallgeier bastianallgeier requested a review from a team April 3, 2025 12:21
@bastianallgeier bastianallgeier force-pushed the v5/fix/remove-null-fields-in-changes branch from 34ad72e to b9a286b Compare April 3, 2025 12:23
@bastianallgeier bastianallgeier mentioned this pull request Apr 3, 2025
5 tasks
@bastianallgeier bastianallgeier linked an issue Apr 3, 2025 that may be closed by this pull request
@bastianallgeier bastianallgeier added this to the 5.0.0-rc.1 milestone Apr 4, 2025
@distantnative
Copy link
Member

distantnative commented Apr 5, 2025

@bastianallgeier It appears to solve part of the problem (image focus can be unset, saved and on reload it remains without focus). However, I am still running in one related problem:

  • Set focus on an image
  • Save
  • Unset the focus (do not save)
  • Reload view
  • Focus is back

I think the problem originates in Panel\Model::content() where Form::for() also merges in the latest content, not just returns the changes content. However, adding merge: false alone doesn't solve the problem as then the form controls stop showing up in the Panel alltogether. Panel\Model::content() probably needs to take care of the situation as well then when the changes version does not exist (as latest isn't anymore merged into it automatically).

@bastianallgeier bastianallgeier force-pushed the v5/fix/remove-null-fields-in-changes branch from 3a7f208 to 9a7723e Compare April 7, 2025 13:44
@bastianallgeier
Copy link
Member Author

@distantnative I've tracked it down yesterday and it's getting more complicated again.

There are three parts to this.

Kirby\Panel\Model::content() should look something like this:

public function content(): array
{
	$version = $this->model->version('changes');

	if ($version->exists('current') === false) {
		$version = $this->model->version('latest');
	}

	// create a form which will collect the latest values for the model,
	// but also pass along unpublished changes as overwrites
	return Form::for(
		model: $this->model,
		props: [
			'values' => $version->content('current')->toArray()
		],
		merge: false
	)->values();
}

This would be very clean, but the problem here is that there might be extra fields in "latest" which will no longer be in the final version. I.e. uuid, slug or template for files. Selectively merging them seems very complicated.

Then we need to work on the changes method in our content.js module. We simply ignore removed fields in there. One option could be another loop:

changes(env = {}) {
    // changes can only be computed for the current view
    if (this.isCurrent(env) === false) {
        throw new Error("Cannot get changes for another view");
    }

    const changes = {};

    for (const field in panel.view.props.content) {
        const changed = JSON.stringify(panel.view.props.content[field]);
        const original = JSON.stringify(panel.view.props.originals[field]);

        if (changed !== original) {
            changes[field] = panel.view.props.content[field];
        }
    }

    for (const field in panel.view.props.originals) {
        if (changes[field] === undefined) {
            changes[field] = null;
        }
    }

    return changes;
},

Then there's still a problematic part in Controller\Changes::save where we use the form again and it will merge the submitted content with the original version once more. I tried to fix it there as well, but failed so far.

It really is still quite a complicated setup.

@bastianallgeier bastianallgeier marked this pull request as draft April 14, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[v5] Image focus can't be removed
2 participants