I'm working on a patch for #2037405: Reacting to file replacement and trying to make sure I understand everything that the existing code is doing.

In _media_derivatives_delete_derivatives() I see the following:

      if (isset($derivative->preset->settings['recursive_delete']) && $derivative->preset->settings['recursive_delete']) {
        $derivative_file = file_load($derivative->derivative_fid);
        _media_derivatives_mark_temporary($derivative_file, $file);
        _media_derivatives_delete_derivatives($derivative_file);
      }

I understand that instead of actually deleting managed files, we mark them as 'temporary' and allow cron to remove them later. What I don't understand is why this is done to managed derivatives only where the preset is set to delete derivatives recursively, but it is done to all unmanaged derivatives. There is no other deletion/temporary marking done to managed files in this function for presets where the 'recursive_delete'. I see the following happening:

  • A video 'example.mp4' is uploaded.
  • Preset A makes derivative 'example-der-a.jpg' which is an unmanaged file.
  • Preset B makes derivative 'example-der-b.wav' which is a managed file, set with 'recursive_delete' == TRUE.
    • Preset C makes a derivative of the wav file, 'example-der-b-der-c.mp3', set with 'recursive_delete' == FALSE.
  • Preset D makes derivative 'example-der-d.3gp' which is a managed file, set with 'recursive_delete' == FALSE.
  • When 'example.mp4' is deleted:
    • derivative 'example-der-a.jpg' is deleted.
    • derivative 'example-der-b.wav' is deleted.
    • derivative 'example-der-b-der-c.mp3' is deleted.
    • derivative 'example-der-d.3gp' is NOT deleted.

This seems like strangely inconsistent behaviour to me. Is that how it is intended to be?

Comments

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

I remember getting some strange race condition if I immediately issued delete of a derivative. I do not remember what exactly was wrong, though (yes.... I should left a comment in there:)).

About unmanaged files. I basically decided to delete them every time as you loose any reference to them if you delete source file. My idea was that you get unmanaged derivatives in source file object when you load it. If you delete source file you basically have no way to get them. In terms of API.

Does this make sense?

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented

Thanks for the reply. Yes, that makes sense. I think in that case, the option of toggling 'recursive delete' should only be available if the file type is 'managed'. When the file type is 'unmanaged' then we should make it clear derivatives will always be deleted (and recursively).

If you agree, I may prepare a patch for this - presumably it would be better to do this separately, rather than in the same patch as my other proposals.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

Sure! Thank you for your hard work.

martin_q’s picture

martin_q
Primary language English
Location Leipzig, Germany | Manchester, UK
CreditAttribution: martin_q commented

No problem. Also, on closer inspection of the code, I think my initial assertion was not all correct. In my example above, I think example-der-b-der-c.mp3 does not get deleted, if it is a managed file with 'recursive_delete' == FALSE. (Just for the record!)