Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2889438-18.patch | 2.56 KB | chr.fritsch |
#18 | 2889438-18-FAIL.patch | 1.47 KB | chr.fritsch |
#18 | interdiff-2889438-12-18.txt | 809 bytes | chr.fritsch |
#12 | interdiff-2889438-10-12.txt | 833 bytes | chr.fritsch |
#12 | 2889438-12.patch | 2.64 KB | chr.fritsch |
Comments
Comment #2
rogerpfaffComment #3
marcoscanoI 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?
Comment #4
phenaproximaMy 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.
Comment #5
seanBI 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.
Comment #6
seanBAdded some docs to explain why we silently do nothing if the icons already exist.
Comment #7
phenaproximaCool beans. RTBC once Drupal CI passes it.
Comment #10
chr.fritschI fixed the file_exists check. It just checked if the folder already exists, not the file.
Comment #11
Wim LeersLooks good!
I wonder if
MediaIconsTest
would be a more appropriate name?inheritdoc
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?
Comment #12
chr.fritschI 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.
Comment #13
seanBI agree with #12. I feel someone else should RTBC, but looks good to me!
Comment #14
Wim LeersWe generally do something like
Not gonna let this nit block a commit.
Comment #15
seanBThanks Wim Leers! Maybe fix it on commit?
Comment #16
Wim LeersYup :)
Comment #17
larowlanCan 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
Comment #18
chr.fritschok, fixed #14 and added fail patch.
Comment #20
seanBLooks good! Thanks.
Comment #21
Wim LeersRTBC++
Comment #23
Gábor HojtsyMakes sense, thanks for adding the explanation to the condition.