Problem/Motivation

Our customers frequently have problems with vanishing files. They upload them, then use them somewhere, then embed them and remove that usage again.

The file is then automatically deleted after 6 hours. That timeframe is now configurable and can be disabled completely but that means that we also no longer delete files that were uploaded in forms but never actually used and e.g. replaced again with another file.

This affects users which use file_entity more than most others, as e.g. media_entity has its own entity type and images there are explicitly kept until manually deleted. But it can also happen just with core, especially as we plan to add an actual media library and not just the useless view that we have now :)

Proposed resolution

A better approach could be to allow users to disable that files are automatically deleted when they are no longer used but were used at some point. That's not perfect either, you possibly want some files to be removed, but it seems like a useful start that is pretty easy to implement.

Core currently doesn't allow to manually delete a file or mark it as temporary , so we might need to postpone this issue on that? (file_entity has bulk operations for that, would be easy to move that to core)

Remaining tasks

None.

User interface changes

We have decided to not make this configuration exposed to the UI:

  • Its hard to explain in general
  • There is the risk of people checking it accidentally and as such loosing files

API changes

None. Behavior change: files that were used at one time (and hence made permanent) are now never deleted, unlike before. It's possible to opt out from the new default though. The new default applies to both existing and new sites.

Data model changes

A key in the file.settings config, plus the corresponding config schema update

Files: 
CommentFileSizeAuthor
#85 interdiff-2801777.txt1.39 KBdawehner
#85 2801777-85.patch14.35 KBdawehner
#82 2801777-82.patch13.88 KBWim Leers
#68 2801777-68.patch17.01 KBWim Leers
#62 interdiff-2801777.txt1.62 KBdawehner
#62 2801777-62.patch16.86 KBdawehner
#60 temporary-files.png25.22 KBifrik
#58 2801777-58.patch16.81 KBJo Fitzgerald
#58 interdiff-56-58.txt4.4 KBJo Fitzgerald
#56 2801777-56.patch16.87 KBJo Fitzgerald
#56 interdiff-54-56.txt4.25 KBJo Fitzgerald
#54 2801777-54.patch17.4 KBMunavijayalakshmi
#49 2801777-49.patch16.78 KBalexpott
#49 21-49-interdiff.txt5.53 KBalexpott
#49 Screen Shot 2017-02-24 at 13.09.47.png131.09 KBalexpott
#47 Screen Shot 2017-02-24 at 12.34.19.png62.02 KBalexpott
#36 2801777-d7.patch3.3 KBPol
#22 21-22-interdiff.txt2.78 KBalexpott
#22 Screen Shot 2016-10-03 at 11.22.42.png89.21 KBalexpott
#21 Screen Shot 2016-10-03 at 10.49.13.png88.39 KBalexpott
#21 2801777-21.patch14.02 KBalexpott
#21 18-21-interdiff.txt11.03 KBalexpott
#19 Selection_078.png12.65 KBBerdir
#19 Selection_077.png29.18 KBBerdir
#18 file-usage-temporary-2801777-18-interdiff.txt2.12 KBBerdir
#18 file-usage-temporary-2801777-18.patch11.73 KBBerdir
#16 file-usage-temporary-2801777-16-interdiff.txt1.42 KBBerdir
#16 file-usage-temporary-2801777-16.patch9.61 KBBerdir
#12 file-usage-temporary-2801777-12.patch9.82 KBBerdir
#12 file-usage-temporary-2801777-12-interdiff.txt1.63 KBBerdir
#10 file-usage-temporary-2801777-10-interdiff.txt2.04 KBBerdir
#10 file-usage-temporary-2801777-10.patch9.82 KBBerdir
#8 file-usage-temporary-2801777-8.patch8.39 KBBerdir
#7 2801777-5.patch568 bytespoornima.n
#4 file-usage-temporary-2801777-4.patch624 bytesBerdir

Comments

Berdir created an issue. See original summary.

slashrsm’s picture

Issue tags: +Novice

+1 for this. Both steps seem to be quite simple (actions + configurability of setting as temporary). Good novice issue candidate?

alexpott’s picture

At the moment we have several bugs where files are being removed when the count is 0 but actually the file is still being used. I think we should consider doing this as a critical mitigation since deleting real content can be unrecoverable whereas disk space usage is a solvable problem.

See #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations and #2666700: User profile images unexpectedly deleted and #2806287: Wysiwyg uploaded images disappear

Berdir’s picture

Status: Active » Needs review
FileSize
624 bytes

Configurable or not configurable?

Could we get bug reports because people replace files and this will no longer delete the old ones?

first patch, just to see how many test fails we have?

Status: Needs review » Needs work

The last submitted patch, 4: file-usage-temporary-2801777-4.patch, failed testing.

dawehner’s picture

IMHO we should make it still configurable, but disable it by default and in an update function.

poornima.n’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.39 KB

Started adding a new setting for this.

Status: Needs review » Needs work

The last submitted patch, 8: file-usage-temporary-2801777-8.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 10: file-usage-temporary-2801777-10.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 12: file-usage-temporary-2801777-12.patch, failed testing.

The last submitted patch, 12: file-usage-temporary-2801777-12.patch, failed testing.

Berdir’s picture

Issue tags: -Novice

I think this is no longer novice :)

Berdir’s picture

the last patch made no sense at all.

dawehner’s picture

IMHO we should add an update path to set the variable onto the settings, and well, my default maybe enable it in the update function? It is a bit of an tricky decision, even well, I think we should default to false.

Berdir’s picture

Berdir’s picture

FileSize
29.18 KB
12.65 KB

update:

the settings is too small to read, click on the link above.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +rc target
  1. +++ b/core/modules/file/config/install/file.settings.yml
    @@ -3,4 +3,4 @@ description:
    +file_usage_temporary: false
    

    I think maybe there's a better name. How about 'make_unused_files_temporary' or 'delete_unused_managed_files'?

  2. +++ b/core/modules/file/file.module
    @@ -1560,3 +1560,29 @@ function _views_file_status($choice = NULL) {
    +function file_form_system_file_system_settings_alter(&$form, FormStateInterface $form_state) {
    ...
    +function file_system_file_settings_submit($form, FormStateInterface $form_state) {
    

    We should test this form alter.

  3. +++ b/core/modules/file/src/FileUsage/FileUsageBase.php
    @@ -10,6 +11,23 @@
    +    $this->configFactory = $config_factory ?: \Drupal::configFactory();
    

    Let's deprecate the automatic getter for Drupal 9.

  4. +++ b/core/modules/file/src/Tests/FileFieldRevisionTest.php
    @@ -22,6 +22,9 @@ class FileFieldRevisionTest extends FileFieldTestBase {
    +
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    
    +++ b/core/modules/file/src/Tests/FileListingTest.php
    @@ -30,6 +30,8 @@ class FileListingTest extends FileFieldTestBase {
     
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    
    +++ b/core/modules/file/src/Tests/FileOnTranslatedEntityTest.php
    @@ -29,6 +29,8 @@ class FileOnTranslatedEntityTest extends FileFieldTestBase {
     
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    
    +++ b/core/modules/file/src/Tests/FilePrivateTest.php
    @@ -26,6 +26,7 @@ protected function setUp() {
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    
    +++ b/core/modules/file/tests/src/Kernel/DeleteTest.php
    @@ -28,6 +28,7 @@ function testUnused() {
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    
    +++ b/core/modules/image/src/Tests/ImageOnTranslatedEntityTest.php
    @@ -29,6 +29,8 @@ class ImageOnTranslatedEntityTest extends ImageFieldTestBase {
     
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    
    +++ b/core/modules/user/src/Tests/UserPictureTest.php
    @@ -33,6 +33,8 @@ class UserPictureTest extends WebTestBase {
    +    $this->config('file.settings')->set('file_usage_temporary', TRUE)->save();
    

    I think we should add a comment for all of these.

  5. If at all possible we should get this into 8.2.0 because this is a very serious bug.
alexpott’s picture

Status: Needs work » Needs review
Issue tags: +String change in 8.2.0
FileSize
11.03 KB
14.02 KB
88.39 KB

re #20
1. After discussion with @Berdir changed it to make_unused_managed_files_temporary
2. Added test
3. Did this.
4. Done

I think we need to work on the UI text because right now with this option the orphaned files setting makes no sense:

alexpott’s picture

After discussing with @xjm here's an improved version for usability. It creates a details element and moves all of the settings inside so users can see that they are related.

Also I've added a warning to the system report if the system is set to remove managed files on 0 usage.

However there are a few problems with the approach here:

  • This change means that there would no way of deleting files from within Drupal
  • Relatedly, this could result in security issues because a user would be unable to remove sensitive content if uploaded.

I'm really not sure about this change because of the above two reasons. That said, the issue of deleting used files is super critical too. This is a hard place to be.

Berdir’s picture

Yeah, what I wrote in the issue summary:

Core currently doesn't allow to manually delete a file or mark it as temporary , so we might need to postpone this issue on that? (file_entity has bulk operations for that, would be easy to move that to core)

arlinsandbulte’s picture

Title: Allow to disable that files which are no longer used are marked temporary automaticaly » Give users the option to prevent drupal from automatically marking unused files as temporary

Title seemed super hard to understand.
I re-worded it to make better sense (I think).
I'm still not sure if it adequately or even correctly describes this issue.

dawehner’s picture

+++ b/core/modules/file/file.install
@@ -120,3 +120,24 @@ function file_requirements($phase) {
+  // Disable deletion of temporary files.

Well strictly speaking this comment does something else now.

webchick’s picture

Don't want to hold up a critical, but just a thought... A global option for this feels a bit clumsy. It seems like it'd be better for you to be able to configure this on a per-filefield/imagefield basis? For example, I might always NOT want to this set on a "Media" field. But DO want to set it on a "PDFs" field, for example.

ifrik’s picture

Discussed during weekly UX hangout, and I offered to do a re-write of the wording based on the discussion.
Two questions:
Q: Is there a difference between "Orphaned" and "unused" here? Apparently not.
Q: Does it need both the tickbox and the "never" in the dropdown? Probably not, but that's a bit of an issue creep.

Berdir’s picture

At the place/point where we remove the last usage, we don't really know anymore where it was initially uploaded. Even for the usage we remove right now, you might have multiple fields on that entity. And you might have re-used a file somewhere and already removed the initial usage.

So, interesting idea, but I don't think we can do that. Maybe if status would not be boolean but allow to separate between "keep in library" and "remove if unused".

Berdir’s picture

Q1: Yes, there is a difference. orphaned is actually temporary. It is about files that already *are* orphaned/temporary and about whether we should delete them when they are. The problem with that approach is that we also don't delete files that were never used (uploaded and then removed again in an node add form for example). Unfortunately, we didn't really think it through when making this configurable :(. The second one about being unused actually refers to persistent, non-temporary files that get their last usage removed and wether we should make them temporary or not.

And once they are temporary, the select configuration kicks in to decide how fast and if we actually delete it.

Q2: Never technically still makes sense, because never also means not deleting any files that were never used or for some other reason marked temporary (e.g. manually in the UI with the operations provided by file_entity.

Does that make sense? Happy to answer more questions.

If I'd tried to describe this in the UI, the description would probably be as long as this comment, good luck making it short and clear :)

Berdir’s picture

That said, I think the explanation above kind of shows that we should recommend to not use never or even deprecate it and mention that in the UI. But that could be a non-critical follow-up issue to not delay this.

slashrsm’s picture

@Berdir is right. I believe that it is currently not possible to do what @webchick suggested in #26. Would be a nice feature though...

It is also worth mentioning that a lot of this issues will go away when/if we introduce the media entity that will be responsible for bringing files into the media library. This will automatically make them used as soon as they are added, which will ensure that they stay there.

Pol’s picture

Hi all,

Here's a first try patch for Drupal 7.x.

Tests are not in yet, I'm busy writing them, but in the meantime, you can give me some feedback.

Status: Needs review » Needs work

The last submitted patch, 32: 2801777-d7.patch, failed testing.

Pol’s picture

Status: Needs work » Needs review
FileSize
3.3 KB

Here's the same patch with green tests.

Pol’s picture

FileSize
3.3 KB

Test with D7 instead of D8.

Pol’s picture

FileSize
3.3 KB

There's a typo in my previous patch.

Here's a fixed patch.

The last submitted patch, 34: 2801777-d7.patch, failed testing.

The last submitted patch, 35: 2801777-d7.patch, failed testing.

Berdir’s picture

@Pol: I could be wrong, but I think the new policy is to have a separate issue for 7.x backport, especially since this hasn't been committed yet to 8.x. See https://www.drupal.org/core/backport-policy

ifrik’s picture

Thanks Bedir,
for the clarifications, but I'm still not sure whether I understand it completely.

1. Does this really only refer to files that are referenced by content? Or are orphaned files not referenced by any entity? For example by user accounts or custom entities.

2. "Orphaned" in general English means something that lost a connection it previously had, something that was meant to last longer, but also something that's orphaned can get adopted again. So for the user of a site an "orphaned file" will only refer to files that still are in the system after the entity that used it was deleted. This makes "orphaned" a specific type of "unused", with "temporary" and "not yet used" as two other types of "unused".
So, if "orphaned" actually means "temporary" then we need to change this wording where ever it shows up sooner then later because it will continue to confuse users, and it probably gets even worse with translations. (But that would be another issue.)

3. If I get it correctly, temporary files get deleted after a given time - unless "Never" is chosen from the drop-down options.

4. If I get it correctly the whole section is about
(a) about setting the deletion period for temporary files, and
(b) about treating unused files as if they were temporary files and delete them accordingly.
If that is correct, then we can get around the whole "orphaned" or so here.

5. I see the reason for swapping the order of the two options in #22, but for any user who is not familiar with the concept will probably first needs to know what happens to the files they could mark before they do so. Therefore I would stick to first setting the time for deletion, and then adding more files to that.

6. I don't think we need one of the warnings because it clearly states "Delete after this time". So the warning basically says "delete means delete".

7. The warning for the deletion time currently advised users not to use this, but comment #30 also proposes to warn not o use never. Which basically means, the user is warned against every option....

So, if I understand all of that correctly, my proposal for wording would be the following.

Temporary and other unused files

Delete temporary files after
Never, 6 hours, 12 hours...
Temporary files are not referenced, but are in the file system and therefore may show up in administrative lists.

[ ] Schedule all unused files for deletion
If enabled, all files that are not referenced will be deleted after the time set above. This will for example remove files that previously were used by content that got deleted. Note: Do not enable this if you intend to upload files for future use, because they might get deleted before they are referenced.
Warning: There are currently known bugs with the counting of file usage. It is recommended to leave this disabled to prevent unintended loss of files.

Berdir’s picture

Uh oh, many questions :)

1. Yes, any entity that could use a file. I meant content as in content entity I think node node/content. our naming is a mess :)

2. not exactly sure. If we say that a file that has no usages left (the last one is removed) is orphaned, then in HEAD, it always becomes temporary. With this new setting, you could have a file that becomes orphaned (=0 usages), but it will *not* become temporary. Deletion is only done for temporary files. So I think we should definitely avoid the use of the term orphaned as it is unclear and could mean something else.

3. Correct.

4. yeah.

5. works for me.

6. ha, yes: "It will be deleted. warning: it really will be deleted. I'm not joking here!". agreed.

7. what we should warn against, IMHO is using "Never" (but as mentioned, maybe that is better discussed in a separate issue). But we want to default to not making files temporary and recommend that users don't change that.

Anyway, your solution looks pretty good to me. this part is tricky: "Temporary files are not referenced". That is more or less true but the reverse isn't. non-referenced files might or might not be temporary. That's basically what the second setting then is about. Whether Non-referenced/unused/orphaned files should be marked temporary. So I think that's not a very good description.

Maybe we could say something like "temporary files are scheduled for deletion.". a bit weird in combination with never because they are scheduled but the scheduled task to delete them actually never runs.

Bojhan’s picture

Issue tags: +Needs screenshots

What should I review here?

Bojhan’s picture

Issue tags: -Needs usability review
Berdir’s picture

Issue tags: -Needs screenshots

The screenshot for the current patch is in #22, combine that with the suggested text improvements in #40 and my feedback on that in #41.

alexpott’s picture

Category: Bug report » Task
Priority: Critical » Major

Recently @catch, @webchick, @xjm, @cottser, @cilefen and I discussed #2801777: Give users the option to prevent drupal from automatically marking unused files as temporary , #2810355: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted and #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations. We all agreed that the current file usage behaviour that is resulting in files being deleted when they are being used by a site is unacceptable and a critical issue. We agree to downgrade and switch this to a major task and create #2821423: Dealing with unexpected file deletion due to incorrect file usage to determine how we are going to handle all the related issues and come up with a plan.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Issue summary: View changes
FileSize
62.02 KB

@ifrik the thing is the terminology "Orphaned files" has been around for ages. Here's a screenshot of the current screen.

I'm not convinced we should be changing that in this issue.

alexpott’s picture

Hmmm. Staring at this some more I think I agree @ifrik - we have no option but to change this here. Because the current text is mis-leading.

alexpott’s picture

Applied most of @ifrik's changes. Not the

Note: Do not enable this if you intend to upload files for future use, because they might get deleted before they are referenced.

As I don't think we don't provide a way to do this in core yet and so it is extraneous information.

Here's a screenshot of the screen with the patch applied.

ifrik’s picture

The additional note is indeed not necessary here. I like the text as it is.

Bojhan’s picture

Temporary files are not referenced, but are in the file system and therefore may show up in administrative lists. Warning: If enabled, temporary files will be permanently deleted and may not be recoverable.

What does "referenced" mean in this case?

swentel’s picture

This could use a backport to D7 as well tbh.

What's left here for getting this in ?

Munavijayalakshmi’s picture

Assigned: Unassigned » Munavijayalakshmi
Status: Needs review » Needs work
Issue tags: +Needs reroll
Munavijayalakshmi’s picture

Assigned: Munavijayalakshmi » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
17.4 KB

Rerolled the patch.

Status: Needs review » Needs work

The last submitted patch, 54: 2801777-54.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
16.87 KB

Fix a couple of bugs in the re-roll.

Status: Needs review » Needs work

The last submitted patch, 56: 2801777-56.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
16.81 KB

I gave up on the patch from #54 (& #56) and went back and re-rolled from #49, hopefully with better results.

Status: Needs review » Needs work

The last submitted patch, 58: 2801777-58.patch, failed testing.

ifrik’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysSeville
FileSize
25.22 KB

The patch applied for me. See screenshot.

About Bojhan's question in #51: "Referenced" means that some entity or content is referring or using the file. We could add that but then it would became a very long list because then we would need to add a full range of whatever can use these files, and it would probably draw the attention to far away from the more important points of this explanatory text.
I think from the usability this is good enough now because it's not a configuration that a novice user is likely to deal with.

Eli-T’s picture

Would this be clearer if instead of

"Temporary files are not referenced, but are..."

the text read

"Temporary files are not currently being used, but are ..." ?

I don't mean to prolong this issue so feel free to disregard.

dawehner’s picture

I think from the usability this is good enough now because it's not a configuration that a novice user is likely to deal with.

While I potentially agree with that, it is a session which can have drastic results, aka. loosing files.
In views we have a yellow background (a drupal_set_message warning style). I wonder whether we could use that kind of warning here as well.

This is a reroll to get it greener.

+++ b/core/modules/file/file.install
@@ -113,6 +115,30 @@ function file_requirements($phase) {
+  $file_config = \Drupal::configFactory()->get('file.settings');
+  $requirements['file_orphaned_file_delete'] = [
+    'title' => t('Orphaned file delete'),
+    'value' => t('There are currently known bugs with file usage counting. It is recommended to leave <a href=":url">\'Schedule all unused files for deletion\'</a> disabled to prevent the loss of files.', [
+      ':url' => Url::fromRoute('system.file_system_settings', [], ['fragment' => 'edit-make-unused-managed-files-temporary'])
+        ->toString()
+    ]),
+    'severity' => $file_config->get('make_unused_managed_files_temporary') ? REQUIREMENT_WARNING : REQUIREMENT_OK,
+  ];

+++ b/core/modules/file/file.module
@@ -1560,3 +1560,41 @@ function _views_file_status($choice = NULL) {
+  $form['temporary_files']['make_unused_managed_files_temporary'] = [
+    '#type' => 'checkbox',
+    '#title' => t('Schedule all unused files for deletion'),
+    '#default_value' => $config->get('make_unused_managed_files_temporary'),
+    '#description' => t('If enabled, all files that are not referenced will be deleted after the time set above. For example, enabling this will delete files that previously were used by deleted content. <strong>Warning:</strong> There are currently known bugs with file usage counting. It is recommended to leave disabled to prevent the loss of files.'),
+  ];

I'm wondering whether we could link to a changer record / issue to let people read more about this.

Wim Leers’s picture

@xjm asked me to look into this. I went through the entire issue.

The following discussions were especially important:

  • @Berdir's initial work taken by @alexpott and pushed forward in #22, approved by @Berdir in #23
  • @webchick's suggestion about making this a per-field setting and how that's not actually possible: #26 + #28 + #31
  • the core committer's stance on this in #45
  • @ifrik's terminology/help text questions + clarifications: #41 + #29 + #30 + #40 + #41, applied by @alexpott in #49, further refining by @Bojhan and @ifrik in #51 + #60

AFAICT what's still necessary is:

  1. issue summary update
  2. change record
  3. rerolled patch
  4. final review

Here's all of those. IS updated. Change record: https://www.drupal.org/node/2891902. Patch attached. Here's a review, I have only one remark/question:

  1. +++ b/core/modules/file/file.install
    @@ -112,7 +114,32 @@ function file_requirements($phase) {
    +  // Disable deletion of temporary files.
    

    s/temporary/unused permanent/

    Fixed.

  2. +++ b/core/modules/file/config/install/file.settings.yml
    @@ -3,4 +3,4 @@ description:
    +make_unused_managed_files_temporary: false
    
    +++ b/core/modules/file/file.module
    @@ -1560,3 +1560,41 @@ function _views_file_status($choice = NULL) {
    +function file_form_system_file_system_settings_alter(&$form, FormStateInterface $form_state) {
    

    Why are we adding a new setting to file.settings.yml?

    Related: why are we doing this using form altering?

    Why don't we add this to system.file.yml and \Drupal\system\Form\FileSystemForm respectively instead?

    The system.file configuration already contains the temporary_maximum_age setting. That seems extremely closely related already?

    (In fact, it's so closely related that this form alter hook actually moves the temporary_max_age setting!)

    This seems like exactly the kind of base infrastructure that belongs in the system module — because anything doing anything file-related should respect this, not just the file module.

Wim Leers’s picture

Only #63.2 still needs to be addressed.

Wim Leers’s picture

Priority: Major » Critical
Issue tags: +data loss

Also moving this back to Critical. This was marked major in #45, in October 2016. That's >8 months ago. All this time, we've been losing files. This is a stopgap solution.

dawehner’s picture

Why are we adding a new setting to file.settings.yml?

Related: why are we doing this using form altering?

Why don't we add this to system.file.yml and \Drupal\system\Form\FileSystemForm respectively instead?

The system.file configuration already contains the temporary_maximum_age setting. That seems extremely closely related already?

(In fact, it's so closely related that this form alter hook actually moves the temporary_max_age setting!)

This seems like exactly the kind of base infrastructure that belongs in the system module — because anything doing anything file-related should respect this, not just the file module.

We should certainly separate the discussion of where something belongs in the UI from the discussion where some setting should be stored. I think we all agree that FileSystemForm is a good place to store it.

A couple of reasons why I believe the current patch is doing the right thing:

  • File usage is a concept of the file module:
    function file_schema() {
      $schema['file_usage'] = [
  • File module is not required, so adding file module specific stuff into system is just wrong.
  • system.file is about stream wrappers right now, which is a different level of abstraction than concrete file entities, which is what file.module is all about

The system.file configuration already contains the temporary_maximum_age setting. That seems extremely closely related already?

The reason why temporary_maximum_age is stored in system module is probably one of the following:

  • Historically it was just like that. System module was just something we threw stuff against
  • function update_delete_file_if_stale($path) {
      if (file_exists($path)) {
        $filectime = filectime($path);
        $max_age = \Drupal::config('system.file')->get('temporary_maximum_age');

    contains a reference to this setting.

dawehner’s picture

#45 decided to move this to Major. I think moving it to critical is totally the right decision. On my talk about best practises in D8 last weekend, I explicitly pointed out this issue as something everyone wants to have on their Drupal installations.

@Wim Leers
There is an open question from the previous comment, which is #60. I think we can move this into a follow up: #2891937: Improve the awareness/usability of disabled temporary file usage cleanup.

+++ b/core/modules/file/src/FileUsage/DatabaseFileUsageBackend.php
@@ -33,7 +34,8 @@ class DatabaseFileUsageBackend extends FileUsageBase {
-  public function __construct(Connection $connection, $table = 'file_usage') {
+  public function __construct(Connection $connection, ConfigFactoryInterface $config_factory = NULL, $table = 'file_usage') {
+    parent::__construct($config_factory);

Can we avoid a BC break by moving ConfigFactory to the end here?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.58 KB
17.01 KB

#66

I think we all agree that FileSystemForm is a good place to store it.

Indeed.

A couple of reasons why I believe the current patch is doing the right thing:

Those are good reasons. Given that, I'm marking this RTBC.

The reason why temporary_maximum_age is stored in system module is probably one of the following:

Indeed, those are the historical reasons. Shall we open an issue to move that setting from system.file to file.settings?

IOW: I'm fine with keeping the new setting in file.settings. But then I think we also should move temporary_maximum_age from system.file to file.settings. If you agree, I'll open an issue for that (or you can if you want to).

#67

I think moving it to critical is totally the right decision. On my talk about best practises in D8 last weekend, I explicitly pointed out this issue as something everyone wants to have on their Drupal installations.

Glad to hear you agree!

Can we avoid a BC break by moving ConfigFactory to the end here?

+1. Done. Those minor/trivial changes shouldn't prevent me from RTBC'ing.

dawehner’s picture

Yeah! I personally would like to see this backported to 8.3, given that this might fix actual production environments.

Indeed, those are the historical reasons. Shall we open an issue to move that setting from system.file to file.settings?

It is tricky given that the usage in update module shows, that places of configuration are sort of a public protocol/our API as well.

Wim Leers’s picture

I personally would like to see this backported to 8.3

+1. But it probably makes sense to get this committed to 8.4.x first?

It is tricky given that the usage in update module shows, that places of configuration are sort of a public protocol/our API as well.

Yes, true. But for the update module we could instead hardcode a number.

dawehner’s picture

Yes, true. But for the update module we could instead hardcode a number.

This is true. There is no reason, at least for me, why this has to be configurable, especially not via the UI.

Wim Leers’s picture

Exactly :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/file/file.install
@@ -112,7 +114,32 @@ function file_requirements($phase) {
+
+    $file_config = \Drupal::configFactory()->get('file.settings');
+    $requirements['file_orphaned_file_delete'] = [
+      'title' => t('Orphaned file delete'),
+      'value' => t('There are currently known bugs with file usage counting. It is recommended to leave <a href=":url">\'Schedule all unused files for deletion\'</a> disabled to prevent the loss of files.', [
+        ':url' => Url::fromRoute('system.file_system_settings', [], ['fragment' => 'edit-make-unused-managed-files-temporary'])
+          ->toString()
+      ]),
+      'severity' => $file_config->get('make_unused_managed_files_temporary') ? REQUIREMENT_WARNING : REQUIREMENT_OK,
+    ];

Should be 'Unused file deletion'.

Also file usage isn't a concept we expose in core, and this doesn't explain the connection to orphaned files. Maybe something like:

"There are currently known bugs with the identification of unused files in core. This can lead to files being deleted even though they're in use. To avoid data loss, disable unused file deletion. Also do we want to skip the link if it's already disabled? Don't see a reason to send people to the form to do the opposite of what we're telling them.

Also I wonder if we really want a UI for this at all - if we made it configurable but left it disabled, then you can disable it if you have to, but there's really no good, safe reason to re-enable it at the moment.

Wim Leers’s picture

at the moment

This is key. We'll want to re-enable its configurability after all child issues of #2821423: Dealing with unexpected file deletion due to incorrect file usage are fixed, if that turns out to be feasible.

dawehner’s picture

This is key. We'll want to re-enable its configurability after all child issues of #2821423: Dealing with unexpected file deletion due to incorrect file usage are fixed, if that turns out to be feasible.

Right, I think having no UI would then actually help, because we can argue better that the user didn't made an explicit choice here to keep it disabled.

Wim Leers’s picture

Wim Leers’s picture

Implemented the suggestions in #73.

Before (always the same text, only different background color)
After (no risk of data loss)
After (risk of data loss)
Wim Leers’s picture

Also I wonder if we really want a UI for this at all - if we made it configurable but left it disabled, then you can disable it if you have to, but there's really no good, safe reason to re-enable it at the moment.

at the moment

This is key. We'll want to re-enable its configurability after all child issues of #2821423: Dealing with unexpected file deletion due to incorrect file usage are fixed, if that turns out to be feasible.

This is key. We'll want to re-enable its configurability after all child issues of #2821423: Dealing with unexpected file deletion due to incorrect file usage are fixed, if that turns out to be feasible.

Right, I think having no UI would then actually help, because we can argue better that the user didn't made an explicit choice here to keep it disabled.

So I'm hearing that we actually want to:

We'd want to do this to protect against data loss. But it'd also mean that sites that have the necessary patches applied still would be forced to leave files around that they'd like to see removed.


I rolled the patch implementing this, then @catch, @dawehner, @Berdir and others can choose whether this is the way to go or not. Then it's at least not blocked on code. No interdiff, because completely different patch.

dawehner’s picture

Personally I thought that this should be kept as a setting, but we just don't expose it in the UI.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Oh! Hah, I misinterpreted, and took it too far then :D Will reroll #77.

Status: Needs review » Needs work

The last submitted patch, 78: 2801777_hardcoded_disabled_deletion-78.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Assigned: Wim Leers » jhedstrom
Status: Needs work » Needs review
FileSize
5.19 KB
13.88 KB

This is then again using the previous approach, interdiff relative to #77 (not #78).

I wasn't sure whether to keep the hook_requirements() implementation. Since 99% of sites do not have the technical props to evaluate their situation/codebase and ensure that only files that should be deleted are actually deleted, I think it's fair to say that this status report entry would be unnecessarily alarmist: there wouldn't be anything the user can do anyway. So, I opted to remove it.

Wim Leers’s picture

Assigned: jhedstrom » Unassigned

Oops.

Bojhan’s picture

No UI, whoooo :)

dawehner’s picture

I wasn't able to find anything beside this test coverage, you know, it cannot hurt to have some.

catch’s picture

Issue tags: -String change in 8.4.0

#85 looks right to me.

dawehner’s picture

Title: Give users the option to prevent drupal from automatically marking unused files as temporary » Prevent drupal from deleting temporary files
Issue summary: View changes

Let's rename the issue title now that we dropped the UI.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Moving to RTBC, but that means I can't commit this without another +1 from someone.

The last submitted patch, 7: 2801777-5.patch, failed testing. View results

Wim Leers’s picture

Wonderful!

mpdonadio’s picture

OK, did a manual test

- Commented out age check in file_cron() so I could test this quickly...
- Create a file filed on basic page
- Create node 1, upload file, copy link into body, save => link in body is OK
- Edit node 1, delete file, save w/o new revision, drush cron => link in body is broken
- Create node 2, upload file, copy link into body => link in body is OK
- Upload file, copy link into body, save => link in body is OK
- Applied #85, ran drush updb, saw updates run
- Edit node 2 , delete file, save w/o new revision, drush cron => link in body is OK
- Create node 3, upload file, copy link into body => link in body is OK
- Upload file, copy link into body, save => link in body is OK
- Edit node 3 , delete file, save w/o new revision, drush cron => link in body is OK

So, I think I

- Verified bug exists
- Verified update hook works
- Verified behavior after update hook

catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +String change in 8.4.0

Committed e3ceb19 and pushed to 8.4.x. Thanks!

Re-tagging string change in 8.4.0 because there's the message in the update that could be translated.

Fixed these on commit:

diff --git a/core/modules/file/file.install b/core/modules/file/file.install
index 0fe7014..48d2bb2 100644
--- a/core/modules/file/file.install
+++ b/core/modules/file/file.install
@@ -5,8 +5,6 @@
  * Install, update and uninstall functions for File module.
  */
 
-use Drupal\Core\Url;
-
 /**
  * Implements hook_schema().
  */
diff --git a/core/modules/file/tests/src/Kernel/UsageTest.php b/core/modules/file/tests/src/Kernel/UsageTest.php
index 88812a1..f95e9cd 100644
--- a/core/modules/file/tests/src/Kernel/UsageTest.php
+++ b/core/modules/file/tests/src/Kernel/UsageTest.php
@@ -77,7 +77,7 @@ public function testAddUsage() {
   /**
    * Tests file usage deletion when files are made temporary.
    */
-  function testRemoveUsageTemporary() {
+  public function testRemoveUsageTemporary() {
     $this->config('file.settings')
       ->set('make_unused_managed_files_temporary', TRUE)
       ->save();
@@ -88,7 +88,7 @@ function testRemoveUsageTemporary() {
   /**
    * Tests file usage deletion when files are made temporary.
    */
-  function testRemoveUsageNonTemporary() {
+  public function testRemoveUsageNonTemporary() {
     $this->config('file.settings')
       ->set('make_unused_managed_files_temporary', FALSE)
       ->save();

  • catch committed e3ceb19 on 8.4.x
    Issue #2801777 by Berdir, Wim Leers, Pol, alexpott, dawehner, Jo...
dawehner’s picture

Issue tags: -String change in 8.4.0

Is there a reason this wasn't committed to 8.3.x?

  • catch committed 37181a6 on 8.3.x
    Issue #2801777 by Berdir, Wim Leers, Pol, alexpott, dawehner, Jo...
catch’s picture

Version: 8.4.x-dev » 8.3.x-dev

No good reason, just a knee-jerk reaction to upgrade paths, but no upgrade path can be worse than random deletion of files. Cherry-picked to 8.3.x too.

xjm’s picture

Issue tags: +8.4.0 release notes
Berdir’s picture

Ouch.

This caused test fails in paragraphs related to entity translation and file usage.

See #2896458: Fix HEAD test fails for file usage and translation. Some were perfectly valid, we checked for files being temporary now, that doesn't happen anymore. Those I fixed already there.

However, there are also a ton of translation related fails that took me a while to even track down to this issue. It complains about node language not what we expect. But that actually makes sense because the step before does not save as it has a validation error.

This is where it gets fun. The validation error is \Drupal\file\Element\ManagedFile::validateManagedFile(). We are not allowed to reference permanent files without usage, as it was assumed that those have been created by non-file-usage managed things like custom uploads. If we'd use them and then no longer use, we'd end up deleting that file which has an unknown, unmanaged origin.

The problem is that our test somehow triggers one of those weird cases where we lose file usages, probably due to the repeated translations being added, removed and language changed. Then we are now longer allowed to save the node because our own file is no longer being used.

Before this patch, this made the file temporary and then probably on save it made it permanent again, so it kind of worked. Now with this committed, there can be cases where it is no longer possible to save an entity if the contained file has no usages anymore due to one of the known or unknown bugs around file usage.

I think as a minimal fix, we should wrap the references check in \Drupal\file\Element\ManagedFile::validateManagedFile() in a check whether this setting is enabled or not, and only check that if unused files are still marked temporary. Not sure if follow-up or revert. I'd kind of hate to revert this as we've been working on this so long but it was also committed to 8.3.x and is causing our tests to fail.

catch’s picture

This is where it gets fun. The validation error is \Drupal\file\Element\ManagedFile::validateManagedFile(). We are not allowed to reference permanent files without usage, as it was assumed that those have been created by non-file-usage managed things like custom uploads. If we'd use them and then no longer use, we'd end up deleting that file which has an unknown, unmanaged origin.
[snip]

Before this patch, this made the file temporary and then probably on save it made it permanent again, so it kind of worked. Now with this committed, there can be cases where it is no longer possible to save an entity if the contained file has no usages anymore due to one of the known or unknown bugs around file usage.

100% agreed here. I'd forgotten we had that validation, but it's yet another reason to move away from file_usage.

Since it looks like the new test failure is exposing one of the other file usage bugs, rather than actually being a new bug, I think it's OK to go forward here - i.e. open a major follow-up to make that validation conditional on this setting.

Berdir’s picture

As discussed, created a follow-up: #2896480: Allow to reference permanent files with 0 usages.

We can work around this in our tests for now by enabling that feature again.

  • catch committed e5f9daa on 8.3.x
    Revert "Issue #2801777 by Berdir, Wim Leers, Pol, alexpott, dawehner, Jo...
xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue tags: +String change in 8.4.0

Per discussion with @catch and @cilefen, we'll leave this as 8.4.x only on account of #2896480: Allow to reference permanent files with 0 usages and the other disruptions, and decide when that's fixed if we want to backport both of them.

dawehner’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Fixed » Needs review

Can we please keep in mind that people are loosing files still in 8.3.x, which is data lass, aka. we should do something against it.
Given that I set it back to 8.3.x

I'm also setting this to 8.3.x and needs review because beside of 5 sites with dropping files, I've not been able to run into #2896480: Allow to reference permanent files with 0 usages

Berdir’s picture

Note that the title change in #87 is not correct. Drupal still deletes temporary files. The point of this issue is to no longer mark unused files as temporary. Not deleting temporary files was already possible but is problematic since that also doesn't delete files that were already deleted. A bit unfortuante that it was already committed, but I'd suggest suggest to change it back.

I'm not 100% sure about the backport to 8.3. I can totally see the argument that it can result in data/file loss, but the thing is that this is a task, not a bugfix. We're not fixing the actual problem, we're just disabling a feature that has existed since at least Drupal 6 (It's controversial feature that especially @catch doesn't like, but still).

For example, we have a few news sites that import a lot of print articles as well as articles from a national news agency. Depending on the specific agreement/licensing some sites publish it automatically, others only after manually changing it and unpublished articles are deleted again after a certain time, sometimes even published content. This results in no longer deleting attached files (mostly images but sometimes also videos), which results in requiring more storage, which costs money and could possibly even result in legal issues if images are still accessible on the site after they should have been removed.

So, all I'm saying is that if we decide to commit this to an 8.3 bugfix release then it should be mentioned *very* prominently in the release notes. And the 8.4 release notes should also mention this clearly, but it is already tagged for those.

dawehner’s picture

Title: Prevent drupal from deleting temporary files » Give users the option to prevent drupal from automatically marking unused files as temporary