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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude created an issue. See original summary.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer commented
FileSize
logo_settings_shown-2620442-2.patch1.58 KB

Test and fix.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer commented
Issue summary: View changes
FileSize
theme settings before.png72.97 KB
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
CreditAttribution: star-szr as a volunteer 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
CreditAttribution: Lendude as a volunteer 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
CreditAttribution: star-szr as a volunteer 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
CreditAttribution: Lendude as a volunteer 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
CreditAttribution: Lendude as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-2620442-2-10.txt494 bytes
logo_settings_shown-2620442-10.patch2.06 KB

This should make the color test pass.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet 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
CreditAttribution: Lendude at Noctilaris commented
Status: Needs work » Needs review
FileSize
logo_settings_shown-2620442-13.patch2.08 KB

reroll, patch didn't apply anymore.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet 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 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three 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
CreditAttribution: Lendude at Noctilaris commented
Status: Needs work » Needs review
FileSize
interdiff-2620442-13-16.txt832 bytes
logo_settings_shown-2620442-16.patch2.09 KB

as requested per #15.

joelpittet’s picture

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

Back to it

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three 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.