fix: generate favourite icon without imagick svg support #55132
Conversation
SebastianKrupinski
force-pushed
the
fix/favourite-icon-without-imagick-svg-support
branch
2 times, most recently
from
e08e5d4 to
23ab2ba
Compare
SebastianKrupinski
force-pushed
the
fix/favourite-icon-without-imagick-svg-support
branch
from
941959d to
aa1d4ef
Compare
SebastianKrupinski
requested review from
a team and
provokateurin
as code owners
SebastianKrupinski
requested review from
come-nc and
salmart-dev
and removed request for
a team
This comment was marked as resolved.
This comment was marked as resolved.
|
@SebastianKrupinski does this address #36607? |
SebastianKrupinski
added
3. to review
SebastianKrupinski
force-pushed
the
fix/favourite-icon-without-imagick-svg-support
branch
from
aa1d4ef to
08869d2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
No, it still uses imagick, but it no longer requires imagick svg support, to generate icons, the icons where generated as png's anyway, it only requires imagick png support and a png logo. But it will generate themed svg icons also If we want to generate themed icons, we have to either limit the input formats to png, and we can use another library like php-gd, or stick with imagick as its the only one that supports svg. Unless we drop the whole themed icons idea then we can drop imagick |
It seems like svg favicons are supported by all major browsers nowadays. I was wondering if it would be possible to have svgs stacked on top of each other by using masks or something for which we dont need to use imagick? |
It's possible, but we would need to find a pure php library or someone would have to write one, then you also need to consider that the input image (logo) would have to be a svg also. |
|
@szaimen are you fine with the current approach? |
Yes |
|
@kesselb @come-nc @provokateurin @salmart-dev 🏓 for review. Thank you 🙏 |
|
|
||
| $response = null; | ||
| $iconFile = null; | ||
| // retrieve instance favorite icon |
There was a problem hiding this comment.
The code already shows that we retrieve the icon. A comment would be more useful if it explained the reasoning or any non-obvious behavior.
There was a problem hiding this comment.
This does explain what we are doing? We are retrieving the user instance configured favorite icon.
|
|
||
| $response = null; | ||
| $iconFile = null; | ||
| // retrieve instance favorite icon |
There was a problem hiding this comment.
| // retrieve instance favorite icon | |
| // retrieve instance favicon |
| } catch (NotFoundException $e) { | ||
| } | ||
| if ($iconFile === null && $this->imageManager->shouldReplaceIcons()) { | ||
| // retrieve or generate app specific favorite icon |
There was a problem hiding this comment.
| // retrieve or generate app specific favorite icon | |
| // retrieve or generate app specific favicon |
There was a problem hiding this comment.
Suggest removing this comment unless there's additional context we should know. Right now it's just echoing the code.
There was a problem hiding this comment.
I would prefer to keep these in, they break up the code into specific logical section and tell us which icon is being worked with. User Instance icon, App specific icon or Generic core icon.
| } | ||
| $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']); | ||
| } | ||
| // fallback to core favorite icon |
There was a problem hiding this comment.
| // fallback to core favorite icon | |
| // fallback to core favicon |
Signed-off-by: SebastianKrupinski <[email protected]>
ChristophWurst
force-pushed
the
fix/favourite-icon-without-imagick-svg-support
branch
from
08869d2 to
69057a5
Compare
apps/theming/lib/Util.php
Outdated
| if ($useSvg) { | ||
| $icon = $appPath . '/img/' . $app . '.svg'; | ||
| if (file_exists($icon)) { | ||
| return $icon; | ||
| } | ||
| $icon = $appPath . '/img/app.svg'; | ||
| if (file_exists($icon)) { | ||
| return $icon; | ||
| } | ||
| } else { | ||
| $icon = $appPath . '/img/' . $app . '.png'; | ||
| if (file_exists($icon)) { | ||
| return $icon; | ||
| } | ||
| $icon = $appPath . '/img/app.png'; | ||
| if (file_exists($icon)) { | ||
| return $icon; | ||
| } | ||
| } |
There was a problem hiding this comment.
| if ($useSvg) { | |
| $icon = $appPath . '/img/' . $app . '.svg'; | |
| if (file_exists($icon)) { | |
| return $icon; | |
| } | |
| $icon = $appPath . '/img/app.svg'; | |
| if (file_exists($icon)) { | |
| return $icon; | |
| } | |
| } else { | |
| $icon = $appPath . '/img/' . $app . '.png'; | |
| if (file_exists($icon)) { | |
| return $icon; | |
| } | |
| $icon = $appPath . '/img/app.png'; | |
| if (file_exists($icon)) { | |
| return $icon; | |
| } | |
| } | |
| $extension = ($useSvg ? '.svg' : '.png'); | |
| $icon = $appPath . '/img/' . $app . $extension; | |
| if (file_exists($icon)) { | |
| return $icon; | |
| } | |
| $icon = $appPath . '/img/app' . $extension; | |
| if (file_exists($icon)) { | |
| return $icon; | |
| } |
|
|
||
| $appIconFile->destroy(); | ||
| return $finalIconFile; | ||
| return false; |
There was a problem hiding this comment.
Did you forgot the catch block for the exception?
There was a problem hiding this comment.
Nope, this is just a default return, this should never get triggered but I did not want to change the function signature. The original code did not throw a error, so just matching that.
There was a problem hiding this comment.
The try-catch-finally block does not catch any exceptions. That means if an error occurs, the variable is unset and the exception still bubbles up. If no exception occurs, the function returns earlier. This makes the final return block unreachable. It is not documented that the functions throw and the previous try-catch block does catch the ImagickException, so I wanted to check whether the catch block is missing.
Signed-off-by: SebastianKrupinski <[email protected]>
|
@kesselb you good with the latest code? |
|
Hey, thanks for picking it up. Themed icon is definitely a rabbit hole ;) I briefly talked about the PR with Christoph. I'm probably not the right person to review or approve this since I don't have enough knowledge about icon generation. I strongly recommend looping in Julius, Barth, or Ferdinand so someone with more expertise can take a look. After reading the code and going through the customer ticket I have a few questions. Some things don't fully add up for me.
|
LOL. Oh, I know, I had to decipher the original code to figure what, when and why
For me this did not work at all, weather this was a logged in user or a non logged in user, so I based my work of that.
The original code, did the same thing, it tried to find the "app specific icon" then the "instance icon" from themes and then as a last resort "stock (shipped) icon".
This was the original logic.
Well the apps should ship both svg and png, I was going to update our apps with both once this was in and working again. Yes we required "imagick with SVG" mainly I think because the original code was using an embedded svg string to generate the background. Also not sure why we forced SVG support when all the icons generated where PNG... the original code always generated PNGs from what I saw, so if the client used a PNG logo and we generated a PNG themed logo why did we need SVG support? On a side note, in my personal opinion, SVG are not safe, they are nice because they are small and scale nicely, but the fact they can contain "links" makes them super scary. |
With SVG support, it does for me 🎨
With the original logic, that path was never executed and went right away to favicon.ico.
Some apps also ship a favicon.png file. Not sure though if we would pick them up somewhere.
cc @juliusknorr @skjnldsv @susnux wdyt? |
No on the long run non-vector SVGs are deprecated. |
Summary
Before
Custom favourite icon was only generate when imagick svg support was turned on
After
Custom favourite is generate even when imagick svg support is not present, as long a a supported image format is used
Testing
TODO
Checklist