Deprecate file_delete() and file_delete_multiple() in file.inc

> Avoid using the deprecated entity_delete_multiple() function.

I don't think it's necessary to not skip deprecated calls in function that are themself deprecated, but this is also fine.

Looks good.

Probably it needs FileLegacyTest to test that deprecated functions still works and trigger expected deprecation messages

I think Drupal\Tests\Core\File\FileSystemDeprecationTest is good one, so it could be introduced here and extended in subissues of main conversion

> FileLegacyTest extends \Drupal\Tests\field\Kernel\FieldKernelTestBase so it seems a bit weird to be putting this stuff there.

I don't think there is a reason for that.

It's a very new test that I copied from \Drupal\Tests\file\Kernel\FileItemTest and didn't update the parent. You can likely just change that, or we can change that elsewhere.

It IMHO makes much more sense there because that's about file entity legacy things. those file functions should never have been in file.inc anyway, they have nothing to do with the FileSystem service.

Drupal\Tests\Core\File\FileSystemDeprecationTest already exists as of #3021458: Properly deprecate drupal_move_uploaded_file() but it's in the wrong place since it's a kernel test.

Please move it to a kernel-flavored namespace and update it there...

Re-rolled and moved the test.

FileSystemDeprecationTest is Drupal\Tests\Core, the functions we are deprecating are in file.module, I still think FileLegacyTest is the best place for this, we just have to fix the parent.

Re: #13: Yes indeedy the deprecation favors the file module, and uses its file entity storage to perform deletions. So these tests are moved there.

That leaves the misplaced Drupal\Tests\Core\File\FileSystemDeprecationTest which is still annoying but can't be piggy-backed onto this issue. Sigh. :-)

This patch reverts FileSystemDeprecationTest and moves the tests from #8 to Drupal\Tests\file\Kernel\FileLegacyTest. Also a minor doc fix.

And the follow-up: #3030359: FileSystemDeprecationTest in the wrong place since it's a kernel test

Looks good, #3028657: Properly deprecate entity_load_multiple() kind of set a standard to deprecate all *_x_multiple() functions at once, not sure if we should do that here as well. many don't exist anymore already, user_delete() (+ multiple) still exists, and quite a few calls to entity_delete_multiple(), so maybe better to them separately.

Setting to RTBC to get feedback on that.

Also removing a bunch of tags as this is not actually about include files.

Oh, but they they actually are in there, just realized again why I specifically suggested to create this issue. That's messed up :)

Rerolled after #3028657: Properly deprecate entity_load_multiple() and #3030359: FileSystemDeprecationTest in the wrong place since it's a kernel test

Re: #15: If you test all of the deprecated functions at once, then you have to expect multiple deprecations at the same time. It's almost a style thing in these cases, but it's better to have the deprecations isolated per test method.

Misunderstanding I think, what I meant is that the other issue deprecated entity_load_multiple(), user_load_multiple(), node_load_multiple() in a single issue, each with its own dedicated legacy/deprecation test. That's what I was wondering, if we should expand this for the other delete functions too.

That said, we we should probably do something similar to \Drupal\Tests\file\Kernel\FileLegacyTest::testEntityLegacyCode() and create an actual file entity that we then delete, e.g. your test would pass if it would not actually pass the correct arguments to $storage->delete()?

I see what you mean... There doesn't seem to be any other test of whether these functions actually do the work they say they'll do.

Yeah, also #2723593: Properly deprecate entity_load() and friends extends the existing test methods for many entity types, so wondering if this should follow that pattern, then you already have the test entity that you then can delete again.

That issue is in, which likely means this conflicts again on changes there, so I'd suggest to add these two function tests also in there.

Reroll.

yes, that's definitely wrong, it was also not merged into the existing entity legacy test method, which I think would make sense with the generic name. it has multiple @expectedDeprecation now already.

Addresses #21 and #24.

Works for me :)

Committed 8d0bc44 and pushed to 8.7.x. Thanks!