Delete media delivered icons on uninstall

I can confirm the bug exists on a clean 8.4.x install.

Should we delete the files when uninstalling, or just override them on re-install?

My suggestion would be that, on a file-by-file basis, we simply do not copy the icons in the first place if they already exist. That way, people can "override" them, in a sense, and we will avoid the possibility of blowing away files that are not ours to touch.

I agree we can't clean up the folder since we don't know which files are placed in the folder, why they are there and how they are used. Als agree with #2, not copying seems the best we can do.

Patch is attached.

Added some docs to explain why we silently do nothing if the icons already exist.

Cool beans. RTBC once Drupal CI passes it.

I fixed the file_exists check. It just checked if the folder already exists, not the file.

1. --- a/core/modules/media/media.install
   +++ b/core/modules/media/media.install
   

   Looks good!

2. +++ b/core/modules/media/tests/src/Functional/MediaInstallTest.php
   @@ -0,0 +1,46 @@
   +class MediaInstallTest extends BrowserTestBase {
   

   I wonder if MediaIconsTest would be a more appropriate name?

3. +++ b/core/modules/media/tests/src/Functional/MediaInstallTest.php
   @@ -0,0 +1,46 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = ['media'];
   

   inheritdoc

4. +++ b/core/modules/media/tests/src/Functional/MediaInstallTest.php
   @@ -0,0 +1,46 @@
   +    // @TODO The following "if" statement can be removed once media is not an
   +    // experimental module anymore.
   

   Can we let this todo point to #2895059: Move media module out of Core (Experimental) package and into the Core package, but mark it hidden?

I addressed 3. and 4. from #11.

Regarding 2:
I think it depends on the scope. These test class is to check that uninstalling and reinstalling is working fine. It's not about testing some specific icon functionality. That's just a part of installing the media module.

I agree with #12. I feel someone else should RTBC, but looks good to me!

+++ b/core/modules/media/tests/src/Functional/MediaInstallTest.php
@@ -37,6 +35,7 @@
     // @TODO The following "if" statement can be removed once media is not an
     // experimental module anymore.
+    // @see https://www.drupal.org/node/2895059

We generally do something like

// @todo Remove this if-statement in https://www.drupal.org/node/2895059

Not gonna let this nit block a commit.

Thanks Wim Leers! Maybe fix it on commit?

Yup :)

Can we get a test only patch please to verify the test does what we expect it to?

Thanks, probably could fix #14 whilst at it if you feel so inclined

ok, fixed #14 and added fail patch.

Looks good! Thanks.

RTBC++

Makes sense, thanks for adding the explanation to the condition.