When media was installed and uninstalled you get an error message the next time you install it.

The file core/modules/media/images/icons/generic.png could not be moved/copied because a file by that name already exists in the destination directory.
The file core/modules/media/images/icons/no-thumbnail.png could not be moved/copied because a file by that name already exists in the destination directory.

Uninstalling should remove those files too or re-installing avoid the error message.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rogerpfaff created an issue. See original summary.

rogerpfaff’s picture

Category: Task » Plan
marcoscano’s picture

Category: Plan » Bug report
Status: Active » Needs review
FileSize
2.07 KB

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?

phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

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.

seanB’s picture

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.

seanB’s picture

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

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Cool beans. RTBC once Drupal CI passes it.

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2889438-6.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
602 bytes

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

Wim Leers’s picture

Status: Needs review » Needs work
  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?

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
833 bytes

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.

seanB’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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.

seanB’s picture

Thanks Wim Leers! Maybe fix it on commit?

Wim Leers’s picture

Yup :)

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

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

chr.fritsch’s picture

The last submitted patch, 18: 2889438-18-FAIL.patch, failed testing. View results

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! Thanks.

Wim Leers’s picture

RTBC++

  • Gábor Hojtsy committed e018da5 on 8.4.x
    Issue #2889438 by chr.fritsch, seanB, marcoscano, Wim Leers, rogerpfaff...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.