Impossible to uninstall any module using the file usage service, because it is impossible to delete all file usages by a module

Well, files can reused no ? So as long as the delete query doesn't blindly delete them, I guess we're ok, unless I'm missing something.

I'm not sure how the criticality of this is justified. Drupal 7 file module did not do any uninstallation of its files either: http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/fi... - although D7 had the file_usage table managed by system module. So does seem to be an existing issue in Drupal 7.

Not sure if the best route is some kind of FileUsageInterface::deleteByModule() or FileUsageInterface::deleteByConditions(array('module' => 'image')), but I would probably say this is major.

I would guess that file module should implement hook_modules_uninstalled() and take care of this for any module that uses file usage so they don't have to worry about it.

Yes, we would need both parts, but at least each module wouldn't have to implement hook_uninstall to remove file usage.

#5 sure - I know there's a gap in the cleaning up of files, I've seen that a lot in D7 sites. However, I thought that another module could also re-use a file from a module that's disabled, but looking at the api, it seems like we're actually safe. So carry on .. :)

Good catch! Patch looks good to me, mark its as RTBC.

Oh, forgot to change issue status.

I thought that your test covers functionality in hook_modules_uninstalled()

This hook covered by tests in system_test_modules_installed() and module_test_modules_uninstalled(), all other modules don't provide test coverage for it. So it seems that no needs in that.

I reviewed this patch and it looks good. It looks like it needs a re-roll though. Asking for a re-test.

#11: module_uninstall_delete_file_usage-2060405-11.patch queued for re-testing.

#11: module_uninstall_delete_file_usage-2060405-11.patch queued for re-testing.

This got committed in http://drupalcode.org/project/drupal.git/commitdiff/a567be3ee03182a7ef09...
I reverted in http://drupalcode.org/project/drupal.git/commitdiff/4ca797aa8c152d7b6a54...

Committed 858674f and pushed to 8.x. Thanks!

Fixed spelling mistake during commit

--- a/core/modules/file/lib/Drupal/file/Tests/UsageTest.php
+++ b/core/modules/file/lib/Drupal/file/Tests/UsageTest.php
@@ -87,7 +87,7 @@ function testDeleteByModule() {

     file_usage()->deleteByModule('testing');

-    $this->assertIdentical(array('file' => array('bar' => array(2 => '2'))), file_usage()->listUsage($file), 'All recors for the "testing" module have been deleted.');
+    $this->assertIdentical(array('file' => array('bar' => array(2 => '2'))), file_usage()->listUsage($file), 'All records for the "testing" module have been deleted.');
   }

   /**

Not exactly sure what the goal of this issue is/was :)

Only *temporary* files with no usage are deleted. And files are marked temporary implicitly when all usages to it are deleted in FileUsageBase::delete(). The patch removes the file_usage entries but obviously doesn't mark them as temporary, so they will stick around forever, without usage information.

Some other things to consider:
- It's not possible to uninstall image.module when there are still any image fields around, removing image fields should also remove the corresponding usages correctly, so the situation for image.module is quite different.
- the file usage stuff used to be in system.module, which couldn't be uninstalled. Now you can uninstall file.module, which means it drops all database tables but doesn't actually delete the files...
- What happens with the embed tags in the content? Unlike image.module, you can uninstall editor.module and they will still be around. If the filter is enabled for some text formats, it will right now explode, with an RTBC issue the behavior will change to not print anything and output watchdog errors, after re-saving the format it will probably just print them out.

I have no idea how to actually solve this. Given the last point, you could argue that the file usage is in fact still there ;)

Well, the system wasn't really designed, file status existed long before the file usage API did I think :) In 7.x, you have to *manually* set the file status when removing/adding file usages. Took me a lot of time & arguments to automate that process in 8.x.

The file usage API right now is kind of optional but not really, when you use the file_managed element #type then it will fail validation when trying to re-save something when there's still file usages.

And the file and file usage tables are now two detached pluggable systems, it's not possible and never would have been performant to replace the status column with an implicit check, this information needs to be materialized. Changing is I guess too much now.

The only idea that I have right now would be that we don't delete immediately (because you can't query something that's not there) but only initiate a process that will then do the cleanup, two possible ways, not sure if they will actually work.

- Instead of deleting the records, set the count to 0. Then implement some code in file_cron() that will look for those and delete them separately, in batches. Would need the ability to search for these records and keep the API introduced here.
- Use the queue API to set-off a self-re-enqueueing process that will search by module, delete N records and if there are more than that, re-enqueue itself. Doesn't need deleteByModule(), only findByModule()

Or, we could of course also remove the ability to uninstall modules :)

Re-opening for now, still not sure if it should be reverted or not.

One possible easy way out:

Only *temporary* files with no usage are deleted.

Is there a good reason for this? If we delete all files with no usage, we are good.

We have no way to query that.

What we do is query for temporary files with a changed date older than 6h (soon configurable), load 100 of them, check if they have any usages, if yes, log a watchdog and skip, otherwise delete.

We can't query for files without usages, because they aren't listed in the file_usage table. And we can't join those two tables as they are both pluggable now.

@Berdir, any reason the file usage service is not a field? Inventing our own storage for everything is so 1999.

Well reason, I guess file usage was added be before files were entities and they're still not fieldable in core, although that's just a flag. Making it a field sounds like a nice idea but too late for that :)

It's not too late for anything. Fixing design issues requires breaking things. That's normal and expected. The API freeze was just premature anyway, given the very sad current state of core.

Reverted with commit 0e47ce3 and pushed to 8.x.

We still need a solution but there is no point leaving a broken solution in.

The issue summary needs updating given that parts of this were committed.

Remove obsolete media tag.