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

When removing all layouts from a flexible, removeCallback from layouts are not called #216

Closed
NorthBlue333 opened this issue Aug 17, 2020 · 3 comments
Labels
bug Something isn't working

Comments

@NorthBlue333
Copy link
Contributor

NorthBlue333 commented Aug 17, 2020

Hi,
I am using Nova Advanced Medialibrary with your package, and I want to delete files not used anymore. I'm currently overriding the method removeCallback to remove those files, but actually the callback is not called if all layouts are removed !
Is this on purpose ?
See the file Whitecube\NovaFlexibleContent\Flexible.php, in the method syncAndFillGroups :

    /**
     * Process an incoming POST Request
     *
     * @param  \Laravel\Nova\Http\Requests\NovaRequest  $request
     * @param  string  $requestAttribute
     * @return array
     */
    protected function syncAndFillGroups(NovaRequest $request, $requestAttribute)
    {
        if (!($raw = $this->extractValue($request, $requestAttribute))) {
            $this->groups = collect();
            return;
        }

        $callbacks = [];

        $new_groups  = collect($raw)->map(function ($item, $key) use ($request, &$callbacks) {
            $layout = $item['layout'];
            $key = $item['key'];
            $attributes = $item['attributes'];

            $group = $this->findGroup($key) ?? $this->newGroup($layout, $key);

            if (!$group) return;

            $scope = ScopedRequest::scopeFrom($request, $attributes, $key);
            $callbacks = array_merge($callbacks, $group->fill($scope));

            return $group;
        });

        $this->fireRemoveCallbacks($new_groups);

        $this->groups = $new_groups;

        return $callbacks;
    }

precisely

        if (!($raw = $this->extractValue($request, $requestAttribute))) {
            $this->groups = collect();
            return;
        }

I am currently overriding this method as a workaround :

    /**
     * Process an incoming POST Request
     *
     * @param  \Laravel\Nova\Http\Requests\NovaRequest  $request
     * @param  string  $requestAttribute
     * @return array
     */
    protected function syncAndFillGroups(NovaRequest $request, $requestAttribute)
    {
        if (!($raw = $this->extractValue($request, $requestAttribute))) {
            $this->groups->each(function ($group) {
                if (method_exists($group, 'fireRemoveCallback')) {
                    $group->fireRemoveCallback($this);
                }
            });
            $this->groups = collect();
            return;
        }

        $callbacks = [];

        $new_groups  = collect($raw)->map(function ($item, $key) use ($request, &$callbacks) {
            $layout = $item['layout'];
            $key = $item['key'];
            $attributes = $item['attributes'];

            $group = $this->findGroup($key) ?? $this->newGroup($layout, $key);

            if (!$group) return;

            $scope = ScopedRequest::scopeFrom($request, $attributes, $key);
            $callbacks = array_merge($callbacks, $group->fill($scope));

            return $group;
        });

        $this->fireRemoveCallbacks($new_groups);

        $this->groups = $new_groups;

        return $callbacks;
    }

I could create a PR to fix this and to add some more usage to the HasMediaLibrary trait if you wish

@toonvandenbos
Copy link
Member

Hi @NorthBlue333,

Thanks for reporting this issue. I did not test your workaround, but why don't you simply do:

if (!($raw = $this->extractValue($request, $requestAttribute))) {
    $this->fireRemoveCallbacks(collect());
    $this->groups = collect();
    return;
}

It avoids code duplication and enhances code readability. Also, we could add some improvements to the fireRemoveCallbacks method in order to make it work without a $new_groups collection as parameter.

Of course, please make a PR for this, it is definitely a bug.

For the other suggested changes in HasMediaLibrary, can you open separated PRs so we don't have to mix subjects? Thanks!

Have a great day.

@NorthBlue333
Copy link
Contributor Author

NorthBlue333 commented Aug 18, 2020

Thanks a lot for your fast answer! Just opened a PR for that.

I'll take a look for another one to update the HasMediaLibrary trait properly. I'm actually running in a segfault error right now and having a hard time figuring out where it comes from... I only know it happens on the line 34 in Laravel\Nova\Http\Controllers\ResourceUpdateController : Nova::actionEvent()->forResourceUpdate($request->user(), $model)->save(); (the actionEvent() is causing it) when trying to upload a file within a flexible using Nova-Media-Library package. Not sure if you can actually help so :)

@toonvandenbos
Copy link
Member

Closed in #217. Thanks @NorthBlue333 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants