Unable to remove last UploadField item in inline editing mode

[kinglozzer]

Module version(s) affected

5.3.0

Description

When inline-editing an item that contains an UploadField, it’s not possible to remove the last item from the UploadField. It appears to be removed, but after saving & refreshing the page it re-appears. Removing other items works as expected, but removing the final item triggers the issue. Affects both multi-upload fields and single item ones.

Screen.Recording.2024-11-04.at.10.08.34.mov

How to reproduce

1. Upload any image(s) to an upkoad field (not clear if it has to be images specifically, haven't tested)
2. Publish the block
3. Refresh the page (may not be necessary)
4. Remove the file(s) from the block
5. Publish the block
6. Refresh the page

Expand to see example elemental block with upload fields

<?php

declare(strict_types=1);

namespace App\Model\Elements;

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\AssetAdmin\Forms\UploadField;
use SilverStripe\Assets\Image;
use SilverStripe\Forms\FieldList;

class ElementLogos extends BaseElement
{
    private static string $table_name = 'ElementLogos';

    private static array $has_one = [
        'Test' => Image::class,
    ];

    private static array $many_many = [
        'Logos' => Image::class,
    ];

    private static array $many_many_extraFields = [
        'Logos' => [
            'SortOrder' => 'Int',
        ],
    ];

    private static array $owns = [
        'Test',
        'Logos',
    ];

    private static array $cascade_deletes = [
        'Test',
        'Logos',
    ];

    private static array $cascade_duplicates = [
        'Test',
        'Logos',
    ];

    private static string $singular_name = 'logos block';

    private static string $plural_name = 'logo blocks';

    private static string $description = 'Logos block';

    private static string $icon = 'font-icon-block-carousel';

    public function getCMSFields(): FieldList
    {
        $this->beforeUpdateCMSFields(
            function (FieldList $fields) {
                $fields->removeByName(
                    [
                        'Test',
                        'Logos',
                    ]
                );

                $fields->addFieldsToTab(
                    'Root.Main',
                    [
                        UploadField::create('Test', 'Test')
                            ->setAllowedFileCategories(IMAGE_SUPPORTED)
                            ->setFolderName('logos'),
                        UploadField::create('Logos', 'Logos')
                            ->setAllowedFileCategories(IMAGE_SUPPORTED)
                            ->setFolderName('logos')
                    ]
                );
            }
        );

        return parent::getCMSFields();
    }

    public function getType(): string
    {
        return 'Logos';
    }
}

Possible Solution

No response

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• ENH Add extension required for new elemental behat test silverstripe-frameworktest#210
• FIX Ensure files can be removed from elemental blocks #1267

[GuySartorelli]

Thanks for reporting this.
Just to confirm, have you tested this in 5.2 (i.e. is this a regression that's new to 5.3.0 specifically)?

[kinglozzer]

Just tested against a site running 5.2 (for both elemental and admin/framework) and this bug does not exist in that version, so this one appears to be a regression

[GuySartorelli]

Looks like the JSON in the POST that saves the block content used to be just nicely the correct types, and in 5.3 all of the values are strings and the file array is simply missing.

I know the code for saving blocks was reworked significantly to get inline-validation working correctly so that's probably the reason it regressed. But I can't think of any reason for the string conversion to be happening.

tl;dr if we can find out why the values are being converted to strings, that's probably also what's preventing the upload field files array from being included in the POST request.

[michalkleiner]

I would probably even bump this to impact/critical as this can seriously impede the content management and there is no workaround.

[GuySartorelli]

Just looking at the labels docs and critical says "Website breaking issue with no workarounds"

I don't know that this qualifies as "website breaking".

Regardless of label semantics, I'll work on a fix for this (or at least try to chase down why those values are being transformed)

[kinglozzer]

You can always remove and re-add the block as a workaround, that’s what I ended up doing 😅

…

On Tue, 5 Nov 2024 at 21:22, Guy Sartorelli ***@***.***> wrote: Just looking at the labels docs <https://docs.silverstripe.org/en/5/contributing/issues_and_bugs/#labels> and critical says "Website breaking issue with no workarounds" I don't know that this qualifies as "website breaking". Regardless of label semantics, I'll work on a fix for this. — Reply to this email directly, view it on GitHub <#1265 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMUF7BHMNMTWY6CGU7S3W3Z7EZK7AVCNFSM6AAAAABRD44FSGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJYGE3TKNJRGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[GuySartorelli]

Looks like formschema has always sent values as strings instead of their original types, and has never sent information about files when you explicitly remove files.

Part of the change to elemental was making it rely on formschema submission directly instead of doing its own funky special form submission. So that explains why the form submission types have changed.

I'm gonna explore two options:

1. see if there's an easy way to make formschema POST submission use original types instead of casting to string
2. if the above isn't feasible, see why other models (e.g. silverstripe/linkfield's FileLink) accept "there's no information about the file" to mean "there's no file" but elemental doesn't.

[GuySartorelli]

1 can't be done, the request is sent using type application/x-www-form-urlencoded
I'll have to try option 2.

[emteknetnz]

Linked PRs have been merged, version of elemental with the patch will be automatically released shortly

[kinglozzer]

Thanks all!