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

CommentFileSizeAuthor
#16 logo_settings_shown-2620442-16.patch2.09 KBlendude
#16 interdiff-2620442-13-16.txt832 byteslendude
#13 logo_settings_shown-2620442-13.patch2.08 KBlendude
#10 logo_settings_shown-2620442-10.patch2.06 KBlendude
#10 interdiff-2620442-2-10.txt494 byteslendude
#3 theme settings after.png44.42 KBlendude
#3 theme settings before.png72.97 KBlendude
#2 logo_settings_shown-2620442-2.patch1.58 KBlendude
logo_settings_shown-TEST-ONLY.patch872 byteslendude

Comments

Lendude created an issue. See original summary.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new logo_settings_shown-2620442-2.patch1.58 KB

Test and fix.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes
StatusFileSize
new theme settings before.png72.97 KB
new theme settings after.png44.42 KB

And some screenies.

The last submitted patch, logo_settings_shown-TEST-ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: logo_settings_shown-2620442-2.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Postponed (maintainer needs more info)

Thanks 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.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@Cottser you can upload a image using the file field and that actually shows? Looking at the code for ThemeSettingsForm::validateForm()

    if ($this->moduleHandler->moduleExists('file')) {
      // Handle file uploads.
      $validators = array('file_validate_is_image' => array());

      // Check for a new uploaded logo.
      $file = file_save_upload('logo_upload', $validators, FALSE, 0);
      if (isset($file)) {
        // File upload was attempted.
        if ($file) {
          // Put the temporary file in form_values so we can save it on submit.
          $form_state->setValue('logo_upload', $file);
        }
        else {
          // File upload failed.
          $form_state->setErrorByName('logo_upload', $this->t('The logo could not be uploaded.'));
        }
      }

The entire upload logic is behind if ($this->moduleHandler->moduleExists('file')). So how can the upload work when that returns FALSE?

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Postponed (maintainer needs more info) » Needs work

Oh 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.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

#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.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2620442-2-10.txt494 bytes
new logo_settings_shown-2620442-10.patch2.06 KB

This should make the color test pass.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Sorry the delay, reviewed the logic and that makes perfect sense. Thanks for catching this bug @Lendude

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: logo_settings_shown-2620442-10.patch, failed testing.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new logo_settings_shown-2620442-13.patch2.08 KB

reroll, patch didn't apply anymore.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Oh that color module caching fix conflicted. Thanks for the quick re-roll

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Nice find...

+++ b/core/modules/system/src/Tests/System/ThemeTest.php
@@ -225,6 +225,18 @@ function testThemeSettingsLogo() {
   }
 
   /**
+   * Tests the dependencies of the theme settings.
+   */
+  function testThemeSettingsDependencies() {

Let's just make this part of the previous test - no need to install drupal all over again

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2620442-13-16.txt832 bytes
new logo_settings_shown-2620442-16.patch2.09 KB

as requested per #15.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Back to it

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 421b476 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 421b476 on 8.1.x 
    Issue #2620442 by Lendude: Theme logo upload settings are shown even if...

  • alexpott committed 265ce06 on 
    Issue #2620442 by Lendude: Theme logo upload settings are shown even if...

Status: Fixed » Closed (fixed)

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