Resolve #3462829 "Custom logo storage"
Closes #3462829
Merge request reports
Activity
added 1 commit
added 43 commits
-
c29fc6c2...5f1bd5ff - 41 commits from branch
project:11.x
- 5ba626cc - Merge remote-tracking branch 'origin/11.x' into 3462829-custom-logo-storage
- 1de0798c - Issue #3462829: Store the file path instead of ID for the navigation logo - Adjust Upgrade path
-
c29fc6c2...5f1bd5ff - 41 commits from branch
added 32 commits
-
c3752670...2fddf325 - 30 commits from branch
project:11.x
- 8eabb0ce - Merge remote-tracking branch 'origin/11.x' into 3462829-custom-logo-storage
- a909fb2d - Issue #3462829: Store the file path instead of ID for the navigation logo - PHPCS
-
c3752670...2fddf325 - 30 commits from branch
added 1 commit
- Resolved by catch
- Resolved by Pablo López
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 } changed this line in version 7 of the diff
Got rid of the post_update hook because the changes made there are now part of
navigation_update_11001()
. Having a mix of update and pst_update hooks could be problematic in specific update scenarios.In this way, sites where the post_update hook was already executed will not be affected and sites updating from previous versions will have changes applied as part of the update hook mentioned above.
- Resolved by catch
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); 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.
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ópezchanged this line in version 10 of the diff
- Resolved by catch
- Resolved by catch
added 1 commit