getkirby / kirby

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

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

Closed nilshoerrmann closed 3 days ago

nilshoerrmann commented 3 weeks ago

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 commented 3 weeks ago

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

nilshoerrmann commented 3 weeks ago

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 commented 3 weeks ago

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.