Skip to content

Conversation

@warleyelias
Copy link
Contributor

@warleyelias warleyelias commented Sep 8, 2022

Description (*)

This PR allows to change both header and small logos by uploading from the Backend. It is much easier than put a image on /skin folder and configure the path in Backend.

I created a validation that allows to continue using the skin image until next logo change, put a default OpenMage logo on default folder for new installation.

Manual testing scenarios (*)

  1. In Backend go to System > Configurations.
  2. In Design > Header section there are two new upload buttons for [Logo Image Src] and [Logo Small Image Src].
  3. Upload the new images and reload the Frontend.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Page Relates to Mage_Page labels Sep 8, 2022
@fballiano
Copy link
Contributor

thanks for you contrib, I think we already have the same PR #1068

@fballiano
Copy link
Contributor

Nowadays we've multiple logos (2x, 3x, mobile, desktop) so my question is, does uploading the logo makes sense today?

@fballiano
Copy link
Contributor

does this allow uploading of SVG logos?

@warleyelias
Copy link
Contributor Author

does this allow uploading of SVG logos?

I didn't test for that file extensions but if not I can change to allow that.

@addison74
Copy link
Contributor

addison74 commented Dec 30, 2022

With #1068 closed I can test this PR. @warleyelias - Please update it for SVG uploads too.

Is there any label that needs translations?

@fballiano
Copy link
Contributor

I would replace the 2 bundled GIF files with just one SVG (which is probably already somewhere in the repo)

@fballiano
Copy link
Contributor

actually I wouldn't bundle another logo image, I'd remove the 2 GIFs and use the SVGs that are already in the repo

@Flyingmana
Copy link
Contributor

@addison74 @fballiano yes it helps, if the logo is managed by people without file access. Maybe even people who have only access to specific websites or storeviews in case of multi national organisations.

@fballiano
Copy link
Contributor

Today in 2023 only having 1 single logo is never sufficient, unless it's an SVG.

The access to a single website/storeview was only available in the enterprise edition.

@Flyingmana
Copy link
Contributor

The access to a single website/storeview was only available in the enterprise edition.

I know of CE shops having this, too

@fballiano
Copy link
Contributor

allowing SVG upload could be more complicated than expected, because of Mage_Core_Model_File_Validator_Image::validate()

@sreichel
Copy link
Contributor

sreichel commented Jan 4, 2023

Can we continue w/o SVG support?

@sreichel
Copy link
Contributor

sreichel commented Jan 4, 2023

It's a feature that doesn't actually help today. I would close it like the other one.

I did not test it (yet), but if it just allows to upload logos from backend ... its a good addition imho.

@Flyingmana
Copy link
Contributor

Can we continue w/o SVG support?

I would say yes, it can be added separately

matteotestoni
matteotestoni previously approved these changes Apr 5, 2023
@fballiano fballiano changed the base branch from 1.9.4.x to main May 15, 2023 20:05
@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Admin Relates to Mage_Admin Component: Cm/RedisSession Relates to Cm_RedisSession ddev documentation environment htaccess Mage.php Relates to app/Mage.php php-cs-fixer labels May 15, 2023
@kiatng kiatng self-assigned this Oct 20, 2025
@kiatng
Copy link
Contributor

kiatng commented Oct 20, 2025

The Delete Image checkbox is broken
image
It doesn't delete the image file.

@sreichel
Copy link
Contributor

Thanks for working on it :)

@github-actions github-actions bot added the translations Relates to app/locale label Oct 21, 2025
@kiatng
Copy link
Contributor

kiatng commented Oct 21, 2025

I have no idea how to fix the PHPMD error. Other than that, I have tested the PR quite a bit, multiple uploads and deletes in various stores. No issue found.

kiatng
kiatng previously approved these changes Oct 21, 2025
@kiatng
Copy link
Contributor

kiatng commented Oct 22, 2025

Before this PR, it's simple to use the same image file in the skin dir as the src for both the main and small logo. With this PR, even though it is BC, but if the user uploads the same image separately to both main and small logo, because the uploader is configured with setAllowRenameFiles(true), so the second upload will be renamed to avoid overwriting the first — resulting in the browser requesting 2 separate resources.

@kiatng kiatng self-requested a review October 22, 2025 07:06
@sreichel
Copy link
Contributor

@all-contributors add warleyelias code

@allcontributors
Copy link
Contributor

@sreichel

I've put up a pull request to add @warleyelias! 🎉

@sonarqubecloud
Copy link

@sreichel
Copy link
Contributor

@kiatng 👍

@sreichel sreichel merged commit 68e07f8 into OpenMage:main Oct 22, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Adminhtml Relates to Mage_Adminhtml Component: Page Relates to Mage_Page new feature PHPStorm translations Relates to app/locale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants