Skip to content
Snippets Groups Projects

Resolve #3462829 "Custom logo storage"

Open Pablo López requested to merge issue/drupal-3462829:3462829-custom-logo-storage into 11.x
2 unresolved threads

Closes #3462829

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 32 32 */
    33 33 function navigation_post_update_set_logo_dimensions_default(array &$sandbox) {
    34 34 $settings = \Drupal::configFactory()->getEditable('navigation.settings');
    35 $settings->set('logo_height', 40)
    36 ->set('logo_width', 40);
    35 if (!$settings->get('logo_height')) {
    36 $settings->set('logo_height', 40);
    37 }
    38 if (!$settings->get('logo_width')) {
    39 $settings->set('logo_width', 40);
    40 }
    37 41 if (is_array($settings->get('logo_managed'))) {
    38 42 $settings->set('logo_managed', current($settings->get('logo_managed')));
    39 43 }
  • Pablo López added 2 commits

    added 2 commits

    Compare with previous version

  • 226 $values = $form_state->getValues();
    227 try {
    228 if (!empty($values['logo_upload'])) {
    229 $filename = $this->fileSystem->copy($values['logo_upload']->getFileUri(), $default_scheme . '://');
    230 $values['logo_path'] = $filename;
    231 }
    216 232 }
    217
    218 $width = $config->get('logo.max.width');
    219 $height = $config->get('logo.max.height');
    220
    221 // Skip if the fid has not been modified.
    222 $fid = reset($logo_managed);
    223 if ($fid == $config->get('logo.managed')) {
    233 catch (FileException) {
    234 $this->messenger->deleteByType(MessengerInterface::TYPE_STATUS);
    • Why is this line necessary? Could maybe use an explanatory comment. It could delete other messages set by the form.

    • Added comment explaining the reason why we have it.

      If the image resize is successful, we previously add a status message indicating it. However, if the copy to permanent storage fails, that message does not makes sense and could be confusing.

      This is an edge case because if the copy fails, the resizing probably would have failed first.

    • Ahh in that case, could we not check the return of $this->adjustLogoDimensions() and only set the success message after reaching this point? Then it wouldn't be necessary to add and subsequently delete a message then.

    • I saw 2 main issues here:

      • adjustLogoDimensions() is invoked during validation, while the file move is done during submit callback. That would imply to pass that information along
      • adjustLogoDimensions() performs the scale, and generates the message because the values are there and returns a boolean. If we want to move this, we might need to recalculate later the new logo dimensions.

      Went ahead and refactored the adjustLogoDimensions() method to return the actual dimensions, and passed the values to the submit callback. Now the message is only shown if the permament storage copy worked.

      Edited by Pablo López
    • Pablo López changed this line in version 10 of the diff

      changed this line in version 10 of the diff

    • Please register or sign in to reply
  • Pablo López added 1 commit

    added 1 commit

    Compare with previous version

  • catch added 1 commit

    added 1 commit

    • e8377b21 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Pablo López added 1 commit

    added 1 commit

    Compare with previous version

  • Please register or sign in to reply
    Loading