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.
Problem/Motivation
Theme logo upload settings are shown even if the file module is not installed. Uploading a file does nothing, no error, no new logo.
Proposed resolution
Fix logic the tests if file module is enabled.
Steps to reproduce:
- Do minimal install
- Go to Appearance settings page
- See the Logo image settings.
Adding failing test. Fix follows.
before
after
Comment | File | Size | Author |
---|---|---|---|
#16 | logo_settings_shown-2620442-16.patch | 2.09 KB | Lendude |
#16 | interdiff-2620442-13-16.txt | 832 bytes | Lendude |
#13 | logo_settings_shown-2620442-13.patch | 2.08 KB | Lendude |
#10 | logo_settings_shown-2620442-10.patch | 2.06 KB | Lendude |
| |||
#10 | interdiff-2620442-2-10.txt | 494 bytes | Lendude |
Comments
Comment #2
LendudeTest and fix.
Comment #3
LendudeAnd some screenies.
Comment #6
star-szrThanks for the report (and patches, and screenshots!) @Lendude.
I can't reproduce the bug here, I was able to change the logo just fine on a minimal install. Is there anything logged in watchdog?
Edit: I should also add that in D7 you can still upload a logo/shortcut icon on minimal with no file module, so we should try to maintain the functionality IMO.
Comment #7
Lendude@Cottser you can upload a image using the file field and that actually shows? Looking at the code for ThemeSettingsForm::validateForm()
The entire upload logic is behind if ($this->moduleHandler->moduleExists('file')). So how can the upload work when that returns FALSE?
Comment #8
star-szrOh when I did a minimal install via drush on D8 it installed Update Manager which needs the File module. When I did the same for D7 it didn't install the File module.
After uninstalling Update Manager and File, I see what you mean.
I think we should first check to see if we can find out why this was changed between D7 and D8.
Comment #9
Lendude#2045189: Move file entity dependent code in includes/file.inc and system.module to file.module is where file_save_upload got moved from file.inc to the file module. So that would be the reason the file module is now required.
Comment #10
LendudeThis should make the color test pass.
Comment #11
joelpittetSorry the delay, reviewed the logic and that makes perfect sense. Thanks for catching this bug @Lendude
Comment #13
Lendudereroll, patch didn't apply anymore.
Comment #14
joelpittetOh that color module caching fix conflicted. Thanks for the quick re-roll
Comment #15
alexpottNice find...
Let's just make this part of the previous test - no need to install drupal all over again
Comment #16
Lendudeas requested per #15.
Comment #17
joelpittetBack to it
Comment #18
alexpottCommitted 421b476 and pushed to 8.0.x and 8.1.x. Thanks!