keepFilesOnDelete not removing all files

[kojot1234]

fixes automatic removal of files with keepFilesOnDelete => false

465

[jorisvaesen]

Looks good to me, what do you think @josegonzalez and @davidyell ?

[josegonzalez]

Seems like the wrong solution. Why not check that the files exist before deleting them? Kind of race-conditiony, but ignoring whether the delete happened successfully seems wrong.

[jorisvaesen]

Hmm looks like a better solution indeed. But there also can be the problem of not having the right permissions to delete a file. This would also exit the delete function and not delete the rest of the files.

[josegonzalez]

Right, so check that the files exist and are deletable before deleting them.

[kojot1234]

@josegonzalez do you see the file check happen within delete function of DefaultWriter or in UploadBehavior before $writer->delete($files); is even called?

[jorisvaesen]

I think the check if there were errors (false returns) should happen outside the foreach loop. This way all files are tried to be deleted and if any fails, false should be returned.

[kojot1234]

Like so?

Return success = true by default or false if any deletion fails.

public function afterDelete(Event $event, Entity $entity, ArrayObject $options)
{
        $result = true;

        foreach ($this->config() as $field => $settings) {
            if (Hash::get($settings, 'keepFilesOnDelete', true)) {
                continue;
            }

            $dirField = Hash::get($settings, 'fields.dir', 'dir');
            if ($entity->has($dirField)) {
                $path = $entity->get($dirField);
            } else {
                $path = $this->getPathProcessor($entity, $entity->get($field), $field, $settings)->basepath();
            }

            $callback = Hash::get($settings, 'deleteCallback', null);
            if ($callback && is_callable($callback)) {
                $files = $callback($path, $entity, $field, $settings);
            } else {
                $files = [$path . $entity->get($field)];
            }

            $writer = $this->getWriter($entity, [], $field, $settings);
            $success = $writer->delete($files);

            if ((new Collection($success))->contains(false)) {
                $result = false;
            }
        }

        return $result;
}

[jorisvaesen]

if ($result && (new Collection($success))->contains(false)) {
    $result = false;
}

No need te create collections when $result is already false.

[kojot1234]

Makes perfect sense here you go