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

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

Open
arneau opened this issue Nov 1, 2023 · 13 comments
Assignees
Milestone

Comments

@arneau
Copy link

arneau commented Nov 1, 2023

  • Laravel Version: 10.30.0
  • Nova Version: 4.29.0

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.

@crynobone crynobone added the pending Issues that are pending triage label Nov 1, 2023
@adshodgson
Copy link

Any update here? same happening for me

@KrendiL101
Copy link

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
Copy link

Any updates or timeline on this?

@jamesblackwell
Copy link

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

@RomainMazB
Copy link

RomainMazB commented Mar 6, 2024

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:

  • The file deletion remains impossible because the null value will resolve to the previous value instead, but you can delete the row, save the form and repopulate the row fields.
  • The repeater ordering is broken: because the form still send null values for the files, each file will be resolved from its previous position in the model attribute. Moving the first row to the second will move the other fields (number, text, etc..) but will keep the file at their initial position.

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

@schniper
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

tonnyorg commented Jun 2, 2024

Looks like I have the same problem: #6410

@crynobone crynobone added this to the 5.x milestone Jun 5, 2024
@jeremynikolic jeremynikolic self-assigned this Aug 7, 2024
@jeremynikolic jeremynikolic added fix incoming A fix is in review and removed pending Issues that are pending triage labels Aug 8, 2024
@tonnyorg
Copy link

tonnyorg commented Sep 1, 2024

Any ETA on this?

@crynobone crynobone added next and removed fix incoming A fix is in review labels Sep 1, 2024
@jeremynikolic
Copy link

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
Copy link

tonnyorg commented Sep 1, 2024

@jeremynikolic ahhh got it, tyvm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests