Skip to content
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

Fix image focus removing #7028

Draft
wants to merge 1 commit into
base: v5/develop
Choose a base branch
from
Draft

Conversation

afbora
Copy link
Member

@afbora afbora commented Feb 24, 2025

Changelog

Fixes

Breaking changes

None

Docs

Ready?

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

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@afbora afbora added this to the 5.0.0-rc.1 milestone Feb 24, 2025
@afbora afbora requested a review from a team February 24, 2025 09:03
@afbora afbora self-assigned this Feb 24, 2025
@afbora afbora linked an issue Feb 24, 2025 that may be closed by this pull request
@distantnative
Copy link
Member

I also saw that this would fix this specific issue.

But I am wondering If this rather points to a general regression, where null/undefined values don't get updated anymore via our new content module. Where in the past they did. @bastianallgeier

@bastianallgeier
Copy link
Member

We should test this in the new holy merge branch

@bastianallgeier
Copy link
Member

I've added unit tests to the "Holy merge" branch to check if we can still remove fields properly by passing null and that works on the backend. If I remember correctly here, the problem was that null values are not properly sent to the server in the first place. I think we should look into that some more.

@afbora afbora marked this pull request as draft February 26, 2025 13:19
@bastianallgeier
Copy link
Member

Ok, I know now where this happens. When a field is unset in the changes version by passing null, the Form class merges it with the latest version and the missing field gets overwritten with the value from the latest version:

return Form::for(
model: $this->model,
props: [
'values' => $changes
]
)->values();

@bastianallgeier
Copy link
Member

I tracked it down further and it seems like the main issue is the Form::for part in ModelWithContent::update because that's another part where we merge the input with the latest version.

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
3 participants