Add 'forceDelete' setting to CouplerBehavior

[gBokiau]

Current behavior :
When deleting the record for a media, if the original file is not found in the transfer directory, record deletion is interrupted without any warning

Proposed fix:
A $forceDelete setting is added to CouplerBeharvior. If set to true, record deletion will proceed even if original media file was not found in baseDirectory. Defaults to false (but does this make sense, see bullet point)

Further thinking:
Should a warning be issued when the file is not found instead or in addition to this behavior ? Would the best approach to this be to use php's unlink() method instead of File::delete() (as unlink() issues an E_WARNING when the file is not found whereas File::delete() doesn't ) ?

• Is there any scenario where it makes sense not to delete the record when the file is not found ? Deleting the record is the prime function of delete(), and should thus proceed whenever possible, while file deletion could be considered an added benefit

[ndm2]

Could you also add tests for this? Generally the whole delete stuff really needs some specific tests, wish I had some time for this.

I think having the possibility to force a delete would generally be useful, and I think it should be disabled by default as it would otherwise break backwards compatibility. The default behavior is generally debatable, but I'd say currently compatibility should be more important.

Should a warning be issued when the file is not found instead or in addition to this behavior ?

Generally issuing a notice, or maybe even a warning for missing files might be an idea, personally, as a developer, I'd like to know about such problems, so I can see how being able to log these kind of problems can be useful even when forcing deletes.

Are you sure the File class causes a warning because the file is missing? I just had a quick look, an I can't see where this would happen.

[gBokiau]

Generally the whole delete stuff really needs some specific tests

You're right, I really lack experience in building test cases, but I'll try to make some time for this

I think it should be disabled by default as it would otherwise break backwards compatibility

True. Perhaps to be revised when we'll need a cake 3.0 rewrite

Are you sure the File class causes a warning because the file is missing?

Sorry, I meant the opposite, 'File' does not, 'unlink()' does. Rephrased the above for clarification

[ndm2]

I have a little bit of free time, so I can add tests for this if you want. I'll add some coupler behavior specific delete tests anyways...

Back to the warning, I think the forceDelete setting should be considered when triggering something at all. In case of enabled forceDelete, one would most probably expect the possibility of files not being deleted, and therefore it's less of a "problem", however when not using forceDelete, a delete failure is somewhat unexpected and might be a little more severe.

So I'd say when file deletion fails it should trigger a warning (E_USER_WARNING) in case forceDelete is disabled, and a notice (E_USER_NOTICE) in case forceDelete is enabled.

[gBokiau]

There are actually a few more possible scenarios : deletion may not work because the file no longer exists or by lack of permission. There's also the problem of race conditions.

Depending on user-scenario you might, or might not, want to alert the user or prompt for his confirmation that the record is to be deleted. (parts of that would probably best be done at validation stage though)

So I think it's best not to make assumptions, but I'm really banging my head on how to impliment a flexible solution. On a hunch I would go for throwing Exceptions — but am lost as to where these should be caught so as to be useable by the app but not stopping execution:

    public function beforeDelete(Model $Model, $cascade = true) {

// .... //

        $file  = $baseDirectory;
        $file .= $result[$Model->alias]['dirname'];
        $file .= DS . $result[$Model->alias]['basename'];

        try {
            if (!@unlink($file)) {
                $error = error_get_last();
                if (strpos($error['message'], 'No such file or directory')) {
                    throw new MediaNotFoundException(array('file' => $file));
                } elseif (strpos($error['message'], 'Permission denied')) {
                    throw new MediaDeletePermissionException(array('file' => $file));
                }
            }
        } catch (Exception $e) {
            if (!$this->settings[$Model->alias]['forceDelete']) {
                return false;
            }
            // How to send Exception $e to the controller
            // while continuing the record deletion  ??
        }
        return true;
    }

… so I guess the most elegant, albeit not the most convenient, solution would be to let unlink() throw its errors and use a custom ErrorHandler reading the messages and outputting appropriate warnings.

Or would there be any other way to let the controller know that the file deletion failed ?

Other thought : @bmcclure created the GeneratedDeletableBehavior to do the same thing with Versions. Would it make more sense to create a new CouplerDeletableBehavior or to add the forceDelete logic to Generator behavior as in gBokiau@1f68997 ?

[ndm2]

You can't pass data back to the controller directly, at least not with the default Model::delete() implementation, as it will only return either true or false, depending on whether the Model.beforeDelete event was stopped (which in turn depends on whether one of the event callbacks returned false). Btw, this is going to change in 3.0, where it will return the result of the event, which is currently being swallowed.

With 2.x, exposing details would for example be possible using events, callback functions, or methods/properties that return/hold information about the errors that occoured, however I'm not sure if I would use exceptions for this, as they seem to be more appropriate for severe problems where something goes so wrong that it's not possible to continue without errors anymore (Printer On Fire 🔥 anyone? ;)). When passing them around like that, then it's not really exception handling anymore, but more like simply error handling where error codes would do it too.

Personally I'm all for exposing details, the question is how much information do you really need in the controller? Permissions, open-base-dir restrictions, file doesn't exist, path isn't a file, etc, wouldn't it maybe be enough to know that the file couldn't not be/wasn't deleted because of a problem whichs details can be found in the logs? Also what about file creation/transfer when creating a record, it's more or less the same problem as it also doesn't return any details about why it failed (apart from validation), but will only trigger errors (in some cases). I think when implementing a more detailed error reporting then it should be as consistent as possible throughout the whole plugin.

[ndm2]

Ah, I've overlooked your latest commit, sorry :)

I kinda like it, though properties on behaviors aren't accessible directly via the model, so one would have to access $Model->Behaviors->Coupler->deleteErrors[$Model->alias], I think exposing a method on the behavior which returns the errors for the current model would be more comfortable.

The exception is pretty drastic, though I can see how it would be helpful to for example halt the process of deleting associated records, and in that case also the deletion of the record itself, that way one wouldn't be left with orphaned associated records, which would be pretty nice actually. However, I'm not sure whether there might be any side effects, at the very last it's not backwards compatible when thrown by default?

[bmcclure]

@gBokiau and @ndm2 -- does it seem like this patch is still the way to go?

[ndm2]

I still like the general idea, however there are still some questions left...