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

Symfony YAML handler stores non-empty empty array value #6637

Closed
nilshoerrmann opened this issue Aug 22, 2024 · 3 comments · Fixed by #6638
Closed

Symfony YAML handler stores non-empty empty array value #6637

nilshoerrmann opened this issue Aug 22, 2024 · 3 comments · Fixed by #6638
Assignees
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Milestone

Comments

@nilshoerrmann
Copy link
Contributor

nilshoerrmann commented Aug 22, 2024

Description

When using the Symfony YAML handler – 'yaml.handler' => 'symfony' – Kirby will create non-empty values for empty content. Given a files field, removing all previously selected files will be stored as [] in the content file. Using the default YAML handler will result in an empty string.

Result with Symfony YAML handler:

Files: []

Result with default YAML handler:

Files:

This divergent behaviour will cause issues when using isEmpty or methods relying on this method (like isNotEmpty or or) because isEmpty checks on the plain field value. So even if isNotEmpty will return true on an object, as consecutive call to toFile() will still result in an empty object. This is unexpected and does break templating logic.

Expected behavior
Empty values should be identified correctly independent of the chosen YAML handler.

Scope
This will affect any field storing collection-like content.

To reproduce

  1. Create a files field.
  2. Select a file, save.
  3. Unselect that file, save.
  4. Check the stored value in the content file.
  5. Repeat with different YAML formatter.

Your setup

Kirby Version
4.4.0 RC1 but it should be the same issue ever since YAML handlers have been introduced.
PHP Version
In case this matters, it happens on PHP 8.2 for us.

@distantnative
Copy link
Member

For the files, pages etc. fields you should better always apply first ->toFiles() etc. and not just apply ->isEmpty() on the raw field.

@nilshoerrmann
Copy link
Contributor Author

nilshoerrmann commented Aug 22, 2024

We usually do that, but in this case we were using the core or method which relies on isEmpty:

        <?php if (
            $image = $plugin->header()->or($plugin->cover())->toFile()
        ): ?>

The fallback was never used because or always assumed the header image to be set und toFile then return null. header and cover both being files fields here.

@nilshoerrmann
Copy link
Contributor Author

For the files, pages etc. fields you should better always apply first ->toFiles() etc. and not just apply ->isEmpty() on the raw field.

Just to make the point: it's Kirby applying isEmpty on the raw field here.

distantnative added a commit that referenced this issue Aug 23, 2024
@distantnative distantnative self-assigned this Aug 23, 2024
@distantnative distantnative linked a pull request Aug 23, 2024 that will close this issue
4 tasks
@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Aug 31, 2024
@distantnative distantnative added this to the 4.5.0 milestone Aug 31, 2024
@distantnative distantnative mentioned this issue Nov 12, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants