_cleanupOldFiles(): using custom dbColumn looks for wrong settings

[mfn]

When $this->_cleanupOldFiles($model, $cleanup); is called from beforeSave(), it is passed the model and a hash where the key is a column name.

This column name is derived from the models settings dbColumn, if present.

In the foreach loop it tries to get the columns settings:

foreach ($fields as $column => $value) {
  if (empty($data[$model->alias][$column])) {
    continue;
  }
  if (!empty($this->settings[$model->alias][$column])) {
    $attachment = $this->_settingsCallback($model, $this->settings[$model->alias][$column]);
    if (!$attachment['cleanup']) {
      continue;
    }
  }

However, it tries to look up the custom column name form dbColumn which has no settings in the models Uploader.Attachment settings:

  var $actsAs = [
    'Uploader.Attachment' => [
      'upload_image' => [
        ...
        'dbColumn' => 'my_custom_column_name',

The lookup performed however is (literal example):

  if (!empty($this->settings[$model->alias]['my_custom_column_name')) {

which won't work; it should be the original and not the mapped column name I think:

  if (!empty($this->settings[$model->alias]['upload_image')) {

This change fixes that.

[milesj]

This seems wrong. The dbColumn value SHOULD be the real name of the column, not an alias or a lookup. There really is no reason to set dbColumn manually as the index of the settings array is the column name. The only reason dbColumn exists is when 2 keys collide for the same column (multiple uploads or transforms, etc).

Your change would actually break most configurations, as it currently works correctly.

[mfn]

Thanks for your feedback, TBH I don't know the code base too well so it was a shot in the dark.

But you wrote:

The dbColumn value SHOULD be the real name of the column

which in my case, it is. But in \AttachmentBehavior::_cleanupOldFiles() when accessing the settings of the model, it is accessing by my custom dbColumn name (which is the real physical column name), but the configuration only exists with the non-real column name.

Comparing with existing code in, here you already doing that $columns[$column] lookup instead of just $column. Please compare https://github.com/milesj/uploader/blob/cb85980ddd05c09d2a9a2212d323e82f058d4e3f/Model/Behavior/AttachmentBehavior.php#L785 :

if (!empty($this->settings[$model->alias][$column] ...

with https://github.com/milesj/uploader/blob/cb85980ddd05c09d2a9a2212d323e82f058d4e3f/Model/Behavior/AttachmentBehavior.php#L473 :

foreach ($this->settings[$model->alias][$columns[$column]]...

In the second place in deleteFiles you're using the column mapping to access the settings, but it's not done in cleanupOldFiles which my PR does.

Maybe I should also iterate why I created the PR: in my configuration I (copied from above with more information):

  var $actsAs = [
    'Uploader.Attachment' => [
      'upload_image' => [
        'dbColumn' => 'my_custom_column_name',
        'cleanup' => false,

I set cleanup to false. But due using dbColumn it is not picked up in _cleanupOldFiles because the lookup into $this->settings ends up in the void. But when I use the column mapping, it finds it.

I tested my code with and without dbColumn and found it to work (but I admit my testing may be too limited).

Thanks

[milesj]

Ah, you're right, my bad. Seems my own column code even confused myself!

Cheers.