Fix proper deletion of files/images within Flexible layouts

[chrisneal]

To get this working:

1. Need to add the AvailableFields trait to resources that use Flexible Layouts.

use \Whitecube\NovaFlexibleContent\Traits\AvailableFields;

2. Use a helper function for the delete callback on file fields.

Image::make('Image')->delete(function(\Laravel\Nova\Http\Requests\NovaRequest $request, $model) {
    return \Whitecube\NovaFlexibleContent\deleteFile($request, $model, $this);
});

[toonvandenbos]

Thanks @chrisneal !

Your approach looks nice. I'll review this tonight.

With your current insights, do you think there could be an easier way for developers to get this to work, even if it requires more work on our side ? I'm just asking in case you'd have more ideas after implementing these changes.

[chrisneal]

@Nyratas I know it's a pain to integrate for users, though this is the only way I can see it working. We're unable to override the DeleteField Nova route or anything, so we need to handle it after that.

The only other thing I can think of would be to create FlexibleImage and FlexibleFile fields, that extend the built-in Image and File fields, with the delete file stuff built in. That would be in addition to the helper function I created and would implement it. What do you think?

[chrisneal]

I've added those...

[chrisneal]

@Nyratas Did you get a chance to look this over?

[toonvandenbos]

Hi @chrisneal,

I don't think we should add those fields in the package. Except adding a custom delete closure, we're not doing anything else on those fields.

Instead, couldn't we parse the Layouts' fields() response and check if it contains File fields, then configure the delete callback on those fields?

[chrisneal]

@Nyratas I don't think it hurts to add them, as they're only helpers and personally I would use them as opposed to having to copy the delete helper everywhere.

I think adding magic (like you're suggesting) could cause more confusion, especially if the user wants to do something themselves with the delete callback.

[toonvandenbos]

@chrisneal I don't agree on the "adding magic" part, as it won't change usage habits : you can use the same fields inside a Layout as you would inside a model's fields() declaration. Having to switch to this package's specific Fields in some cases adds complexity.

I however agree on the fact that we should let the ability to define a custom delete callback.

Therefore, when applying the "magic" deleteFile callback on those fields, we should check if they already had a defined callback. If so, our deleteFile would just "wrap" that callback, parse the data that needs transformation, then pass the updated parameters to the user defined callback for further treatment.

When there is no user callback defined, we could switch back to default behavior.

Something like:

// We're somewhere in a loop that's parsing the defined layout File fields...

if(is_callable($field->deleteCallback)) {
    $callback = $field->deleteCallback;
    $field->delete(function(NovaRequest $request, $model) use ($callback) {
        return \Whitecube\NovaFlexibleContent\wrapDeleteFile($callback, $request, $model, $this);
    });
} else {
    $field->delete(function(NovaRequest $request, $model) {
        return \Whitecube\NovaFlexibleContent\defaultDeleteFile($request, $model, $this);
    });
}

[toonvandenbos]

Of course this is a quick and dirty example of what I mean. Since the deleteFile helper will have more complexity it may be a better idea to move it into a separate class that can handle the "default" and the "wrapped" behavior. Something like PerformFileDelete or similar. What do you think?

[chrisneal]

@Nyratas I've updated the branch to automatically add the delete callback. I don't particularly like the placement of the code, but it is the best place for it IMO.

I've also removed the 2 helper files.

[chrisneal]

@Nyratas You had a chance to test this?

[toonvandenbos]

@chrisneal I took a look at your code. I will merge this PR but since I have quite a lot of changes to request, I will add those myself before publishing the release. Thanks for your work !

[chrisneal]

@Nyratas I'd be interested to know what changes you want to make. But glad you're going to merge it.

[chrisneal]

@Nyratas FYI, I've found a couple of bugs in my code since we last had a conversation. Any idea when you're planning on creating a release?

[chrisneal]

@Nyratas You gonna merge this one soon?

[flxsource]

@Nyratas is there much more to do on this PR (other than rebasing onto the latest changes)? Anything we can assist with?

[voidgraphics]

@chrisneal @FixSource Thank you for your patience and sorry for the inactivity, we had a very difficult deadline for a client. I think @Nyratas wanted to refactor most of the code in this PR, because I recall he found some tricky problems with it. Hopefully he can get to it soon or explain what should be done to get it in shape to merge. Rest assured we are not forgetting about this and intend to get to it as soon as we can.

[NioTeX]

Hey guys, any update on this?

[NioTeX]

Anything we can maybe help with to get this merged?

[sweebee]

any update on this?

[GarethSomers]

I don't have this issue but in meantime...

I solved a similar problem in another PR (merged) for by adding to Whitecube\NovaFlexibleContent\Layouts\Layout the method removeCallback which is called when a Layout is removed. This might be helpful to clean up any Media / etc.

class SomeLayout extends Layout
{
    /**
     * The default behaviour when removed
     *
     * @param $flexible The $flexible field
     * @param Whitecube\NovaFlexibleContent\Layout $layout The layout (should equal $this).
     */
    protected function removeCallback($flexible, $layout)
    {
        // Delete images manually?
    }
}

[tuimz]

What is the status on this? Can this be rebased and merged in the mean time :)

[flxsource]

@voidgraphics @Nyratas Is there going to be any movement on this issue? I'm happy to fork this and apply this fix if needed, but wanted to check that this isn't going to be imminently addressed first.

[jasperlanting10]

Any information on this issue and the possible merge of the fix?

[ajitya2002]

Any update when this issue will be fix?