laravel / nova-issues

556 stars 34 forks source link

Image field "values" don't persist on update after initial upload when used with a Repeater #6015

Open arneau opened 1 year ago

arneau commented 1 year ago

Description:

As per this issue's title, it appears as though Image field "values", when used in conjunction with Repeater fields, don't persist. I realise that Repeater fields are still in beta, but don't see anything in the documentation at https://nova.laravel.com/docs/resources/repeater-fields.html covering that.

My table structure is as follows ...

products
- id

product_images
- id
- description
- image
- product_id

My classes are as follows ...

app/Models/Product.php:

<?php

namespace App\Models;

...

class Product extends Model
{
    ...

    public function images()
    {
        return $this->hasMany(ProductImage::class);
    }

    ...
}

app/Models/ProductImage.php:

<?php

namespace App\Models;

...

class ProductImage extends Model
{
}

app/Nova/Product.php:

<?php

namespace App\Nova;

...

class Product extends Resource
{
    ...

    public function fields(NovaRequest $request)
    {
        return [
            ...
            new Panel('Images', [
                Repeater::make('Images')->repeatables([
                    ProductImage::make(),
                ])->asHasMany(),
            ]),
            ...
        ];
    }

app/Nova/Repeater/ProductImage.php:

<?php

namespace App\Nova\Repeater;

...

class ProductImage extends Repeatable
{
    public static $model = \App\Models\ProductImage::class;

    public function fields(NovaRequest $request)
    {
        return [
            ID::make(),
            Text::make('Description')->rules('max:255'),
            Image::make('Image')->deletable(false)->disk('public'),
        ];
    }
}

Detailed steps to reproduce the issue on a fresh Nova installation:

  1. Add a product image (repeatable) ... Screenshot 2023-11-01 at 14 25 39

  2. Select image (file) ... Screenshot 2023-11-01 at 15 09 28

  3. Update product (model) ... Screenshot 2023-11-01 at 14 27 38

  4. Confirm product_images.image column contains correct value ... Screenshot 2023-11-01 at 15 10 54

  5. Update product (model) again ... Screenshot 2023-11-01 at 15 11 39

  6. Notice the product_images.image column no longer has a value ... Screenshot 2023-11-01 at 15 12 15

In closing

I suspect that the image column's value is empty after subsequent saves because those rows are deleted and recreated (as per https://nova.laravel.com/docs/resources/repeater-fields.html#upserting-repeatables-using-unique-fields), for some reason the original value isn't included in the XHR payload, and it looks like that image attribute will therefore only be "filled" if a new file is uploaded.

Screenshot 2023-11-01 at 15 21 14

I have been able to get that image column's values to persist by specifying a unique field so that upserts are run instead (ie. ->uniqueField('uuid')), but then the reordering of images (repeatables) no longer work.

adshodgson commented 11 months ago

Any update here? same happening for me

KrendiL101 commented 10 months ago

It is still not resolved, can't save image in repeatable after first save. Also, when saving two images in repeatable, the same error

christian-heiko commented 9 months ago

Any updates or timeline on this?

jamesblackwell commented 9 months ago

Bouncing this, just purchased a new Nova license and this still seems to be a bug

RomainMazB commented 8 months ago

Absolutely not a final solution but I've searched where the issue was and it's kind of complex.

My use case is using the JSON preset and to me, the issue come from the fact that a clean Fluent is created inside of the preset, meaning that the File field can't keep track of a precedent eventual value of the data.

ATM for those who really need to keep files data after an update request, I've found this temporary solution:

I created a custom File field and JSON preset to modify these few lines

// In JSON::set method, replace the $callbacks variable initialization by this one
$callbacks = $fields
    ->withoutUnfillable()
    ->withoutMissingValues()
    ->map(function (Field $field) use ($model, $attribute, $request, $requestAttribute, $data, $blockKey) {
        $originalAttribute = $model->getOriginal($attribute);
        if (!is_null($originalAttribute) && isset($originalAttribute[$blockKey])) {
            $field->resolve($originalAttribute[$blockKey]['fields'], $field->attribute);
        }

        return $field->fillInto($request, $data, $field->attribute, "{$requestAttribute}.{$blockKey}.fields.{$field->attribute}");
    })
    ->filter(function ($callback) {
        return is_callable($callback);
    });
// In File::fillAttribute method, move up the $hasExistingFile initialization and add this condition
protected function fillAttribute(NovaRequest $request, $requestAttribute, $model, $attribute)
{
    // This line was just moved from a later section of the method to be used earlier
    $hasExistingFile = ! is_null($this->getStoragePath());

    if (is_null($file = $this->retrieveFileFromRequest($request, $requestAttribute))) {
        // Add this condition to fill the attribute from the initial now-known value
        if ($hasExistingFile) {
            return $model->{$attribute} = $this->getStoragePath();
        }

        return;
    }
    // Leave the rest unchanged

I've created a gist here to see the full code example. Please share if you succeed further than me :)

This adds more support for the File field and if needed you can override its descendant to make extends your File field class for Audio, Image, Avatar and so on... as needed.

If you dont want to override a bunch of classes, you can even make this fix "like native" with this composer trick to load your File and JSON preset instead of the vendor one.

You won't lose your file paths on updating anymore, but there is remaining cons to this temporary solution:

Hoping that get fixed by a proper solution quickly 🤞🏻.

schniper commented 8 months ago

Here is my approach, which doesn't require modifying other files and may even be easy to integrate in the framework:

Add a hidden field to your Repeatable: Hidden::make('Key') ->default(fn () => uniqid()),

In your model, do something on the lines of `protected static function boot() { parent::boot();

    static::updating(function ($model) {
        $originalDocuments = Arr::keyBy($model->getOriginal('documents'), 'fields.key');

        $model->documents = Arr::map($model->documents, function ($document) use ($originalDocuments) {
            if (! empty($document['fields']['file'])) {
                return $document;
            }

            if (! isset($originalDocuments[$document['fields']['key']])) {
                return $document;
            }

            $document['fields']['file'] = $originalDocuments[$document['fields']['key']]['fields']['file'] ?? '';

            return $document;
        });
    });
}`

This assumes my DB json field is called documents, and my repeatable has a file field called "file", which I want to retain. As it probably is obvious, I am using those generated keys to put back the file paths, when updating the model.

AbdullahGhanem commented 6 months ago

Fixed by this section Link

just add ->uniqueField('id')

Repeater::make('Line Items')
  ->asHasMany()
  ->uniqueField('id')
  ->repeatables([
  \App\Nova\Repeater\LineItem::make()
  ])

and add ID::hidden(), // The unique ID field

KrendiL101 commented 6 months ago

Fixed by this section Link

just add ->uniqueField('id')

Repeater::make('Line Items')
  ->asHasMany()
  ->uniqueField('id')
  ->repeatables([
  \App\Nova\Repeater\LineItem::make()
  ])

and add ID::hidden(), // The unique ID field

This is working perfect, thanks!

philipdowner commented 6 months ago

Fixed by this section Link

just add ->uniqueField('id')

Repeater::make('Line Items')
  ->asHasMany()
  ->uniqueField('id')
  ->repeatables([
  \App\Nova\Repeater\LineItem::make()
  ])

and add ID::hidden(), // The unique ID field

Agree that this works when using a distinct model via the asHasMany() method, but the issue still persists when storing the data as JSON. For example, my model has an images JSON column, and the repeater allows the storage of path, caption, and alt_text fields.

tonnyorg commented 5 months ago

Looks like I have the same problem: #6410

tonnyorg commented 2 months ago

Any ETA on this?

jeremynikolic commented 2 months ago

Hi @tonnyorg, we are working on the next version, Nova 5 which will contain this fix. No set ETA yet but very soon, which is why we decided not to add beta related fixes to 4👍

tonnyorg commented 2 months ago

@jeremynikolic ahhh got it, tyvm!