Improve Dockerman icon caching.#1146
Conversation
| global $docroot, $dockerManPaths; | ||
| $imgUrl = $this->getTemplateValue($Repository, 'Icon','all',$contName); | ||
| if (!$imgUrl) $imgUrl = $tmpIconUrl; | ||
| $imgUrl = $iconUrl ?: $this->getTemplateValue($Repository, 'Icon','all',$contName); |
There was a problem hiding this comment.
Prefer iconUrl specified by container label if it exists.
| $iconRAM = sprintf('%s/%s-%s.png', $dockerManPaths['images-ram'], $contName, 'icon'); | ||
| $icon = sprintf('%s/%s-%s.png', $dockerManPaths['images'], $contName, 'icon'); | ||
| $imgUrlHash = sha1($imgUrl); | ||
| $iconFile = sprintf('%s-%s.png', 'icon', $imgUrlHash); |
There was a problem hiding this comment.
Instead of storing the icons by container name only, this change uses a sha1 hash of the imgUrl. This way a change to the icon url will automatically invalidate the cached icon.
| $iconFile = sprintf('%s-%s.png', 'icon', $imgUrlHash); | ||
|
|
||
| $iconRAM = sprintf('%s/%s-%s', $dockerManPaths['images-ram'], $contName, $iconFile); | ||
| $icon = sprintf('%s/%s', $dockerManPaths['images'], $iconFile); |
There was a problem hiding this comment.
In images-ram the icon file name contains both the container name and the urlHash so that it is unique for each container.
In images the icons are store by urlHash alone. This has the benefit of storing only a single copy of identical icons.
| return (is_file($iconRAM)) ? str_replace($docroot, '', $iconRAM) : ''; | ||
| } | ||
|
|
||
| public function purgeUnusedIconFiles($contName, $keepIcon='') { |
There was a problem hiding this comment.
This new function better handles the more complex removal process for this caching mechanism.
| if ( ($keepIcon === '') || !(strpos($filename, $keepIcon) !== false) ) { | ||
| @unlink($filename); | ||
| } | ||
| } |
There was a problem hiding this comment.
The first block removes all icons associated with contName in images-ram except the one specified by keepIcon.
| @unlink($filename); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The next block removes all icons stored in the old style from the images directory. Ideally this would only need to be run once when the caching mechanism was first upgraded to purge all old icons. Unfortunately i could not think of a good way to make that happen so i settled for always running this check.
| @unlink($filename); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The final block removes any of the shared icons in images that share the same hash as icons removed in the first block that are not referenced by another icon in images-ram.
| $imgUrl = $iconUrl ?: $this->getTemplateValue($Repository, 'Icon','all',$contName); | ||
| if (!$imgUrl || trim($imgUrl) == "/plugins/dynamix.docker.manager/images/question.png") return ''; | ||
|
|
||
| $imageName = $contName ?: $name; |
There was a problem hiding this comment.
I removed this line specifically because i could not figure out what it was doing. So far as i can tell $name is never set anywhere.
| $iconPath = $DockerTemplates->getIcon($Repository,$Name); | ||
| @unlink("$docroot/$iconPath"); | ||
| @unlink("{$dockerManPaths['images']}/".basename($iconPath)); | ||
| $DockerTemplates->purgeUnusedIconFiles($Name); |
There was a problem hiding this comment.
Since removing an icon from the cache is a bit more complex I use a new function specifically for that purpose.
|
My unRAID server has ~42 containers running with about ~30 unique icons. In my testing the changes to the caching mechanism do not seem to greatly effect the execution time of the getAllInfo function. |
| if ($ct['Icon']) $tmp['icon'] = $ct['Icon']; | ||
| if ( ! $communityApplications ) { | ||
| if (!is_file($tmp['icon']) || $reload) $tmp['icon'] = $this->getIcon($image,$name,$tmp['icon']); | ||
| if (!is_file($tmp['icon']) || $reload) $tmp['icon'] = $this->getIcon($image,$name,$ct['Icon']); |
There was a problem hiding this comment.
If this change is not made then subsequent calls to getAllInfo will invalidate the cache by passing in the icon file path via tmp['icon'] instead of the icon url.
|
It'd be great to get some eyes on this whenever someone has time - I've got between 50 and 60 containers on each of my unraid servers, and loading the docker tab's gotten to feeling pretty clunky at this point, so any performance improvement would be great! |
dcflachs
force-pushed
the
dockerman_icon_patch
branch
from
0365a1e to
a5bd646
Compare
|
Rebased changes onto latest rev |
|
I would love to get some feedback on this issue and proposed fix. |
|
Not my call, but since this is from 2022 you might want to retest the base system on 7.x and resubmit (even though there are no merge conflicts). Quite probable that since it is a very old PR that it's not going to get looked at since there have been numerous changes in DockerClient. |
WalkthroughConsolidates Docker icon handling: getAllInfo now tracks label icon URL changes and calls getIcon with a prefer-flag; getIcon computes SHA1-based filenames and RAM/persistent paths, ensures RAM icons, and triggers purgeUnusedIconFiles to remove stale icons; CreateDocker now delegates icon cleanup to purgeUnusedIconFiles. ChangesDocker icon handling
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateDocker
participant DockerTemplates
User->>CreateDocker: Submit container create/update
CreateDocker->>DockerTemplates: purgeUnusedIconFiles(Name)
CreateDocker->>DockerTemplates: getIcon(repository, name, iconUrl, prefer)
DockerTemplates->>DockerTemplates: resolve iconUrl → sha1 filename
DockerTemplates->>DockerTemplates: download/copy icon to RAM
DockerTemplates->>DockerTemplates: purgeUnusedIconFiles(name, keepIcon)
DockerTemplates->>CreateDocker: return icon path
CreateDocker->>User: Complete create/update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
347-376: 🛠️ Refactor suggestionAvoid “only variables should be passed by reference” notice
Inside
purgeUnusedIconFiles()the suffix is extracted with:$suffix = end(explode('-', $ramFile));
end()expects its argument by reference; passing the result ofexplode()triggers
Only variables should be passed by referenceon PHP 7+ (and still emits a notice on earlier versions).
Assign to a variable first:- $suffix = end(explode('-', $ramFile)); + $parts = explode('-', $ramFile); + $suffix = end($parts);This eliminates the notice and is forward-compatible with PHP 8’s stricter reference handling.
🧹 Nitpick comments (3)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
85-91: Centralised icon-cleanup call looks correct, but consider preserving the current icon when possibleReplacing the hand-written
unlink()sequence with$DockerTemplates->purgeUnusedIconFiles($Name)is a nice consolidation and keeps the template layer in charge of file housekeeping.
One small follow-up: at this point you already know the hash-based filename thatgetIcon()will eventually keep (it’sicon-<sha1>.png) once the new icon is downloaded. If you pass that value as the second argument you avoid a brief window where both the RAM and persistent copies are removed, forcing a re-download on the next page refresh:$keep = sprintf('icon-%s.png', sha1($_POST['contIcon'])); $DockerTemplates->purgeUnusedIconFiles($Name, $keep);Not strictly required, but it shaves an extra HTTP request for users who reopen the Docker tab immediately after clicking Apply.
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
355-364: Directory creation race – use@mkdirwith the recursive flag
mkdir(dirname($iconRAM), 0755, true)is fine when multiple concurrent calls are unlikely, but if twogetIcon()invocations land at the same time the second one will throw a warning because the directory now exists.
You’re already suppressing IO warnings elsewhere, so consider the same pattern here:- if (!is_dir(dirname($iconRAM))) mkdir(dirname($iconRAM), 0755, true); - if (!is_dir(dirname($icon))) mkdir(dirname($icon), 0755, true); +@mkdir(dirname($iconRAM), 0755, true); +@mkdir(dirname($icon), 0755, true);It keeps the intent while removing a harmless but noisy warning in the logs.
378-406: Pattern mismatch may leave orphaned persistent icons
purgeUnusedIconFiles()deletes persistent icons using the glob pattern
%s/%s*.png(i.e.<contName>*.png).
Since persistent icons are now stored asicon-<sha1>.pngwithout the container name, they are never caught by this first pass and rely solely on the suffix-cleanup logic below.That works, but it’s indirect and means the initial
glob()does unnecessary work.
You could simplify (and speed up) the method by deleting from the persistent directory based on the computed$keepIconinstead:- $icon_glob = sprintf('%s/%s*.png', $dockerManPaths['images'], $contName); + $icon_glob = sprintf('%s/icon-*.png', $dockerManPaths['images']);and keep the “suffix” reconciliation for safety.
Not a correctness bug, but trimming the extra filesystem pass reduces IO when many containers share the same icon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
purgeUnusedIconFiles(378-406)
|
Bump. Really need this. |
|
I agree. I didn't know what was happening when I added a second container of the same image and they had 2 different icons but came from the same registry. Even if there was a way to only do it via the command line, that would work for me |
elibosley
force-pushed
the
dockerman_icon_patch
branch
from
a5bd646 to
2f8ebfa
Compare
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php`:
- Line 213: The exec() calls in CreateDocker.php construct shell arguments by
concatenating extra single quotes around escapeshellarg() results, causing
doubled quotes like ''value''; locate the exec(...) invocations that build the
docker command (the ones using --name and the repository/tag variables, e.g.,
where $Name and $Repository/$Tag are passed) and remove the outer single-quote
concatenation so you pass the raw escapeshellarg($var) return value (which
already includes quotes) into the command string for all four occurrences.
- Line 502: The exec call in CreateDocker.php concatenates $xml['Name'] directly
into the shell command causing a command-injection risk; sanitize/escape the
container name before invoking exec by replacing the concatenation with a safely
quoted argument (e.g., use PHP's escapeshellarg on $xml['Name']) or avoid a
shell invocation entirely (use a direct API or proc_open with argument arrays)
so the exec("docker exec -i ... /bin/sh -c \"tailscale exit-node list\"") call
no longer passes untrusted input to the shell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec03d287-c535-494c-9804-a3ff03f01847
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpemhttp/plugins/dynamix.docker.manager/include/DockerClient.php
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.docker.manager/include/DockerClient.php
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php`:
- Line 213: The exec() calls in CreateDocker.php construct shell arguments by
concatenating extra single quotes around escapeshellarg() results, causing
doubled quotes like ''value''; locate the exec(...) invocations that build the
docker command (the ones using --name and the repository/tag variables, e.g.,
where $Name and $Repository/$Tag are passed) and remove the outer single-quote
concatenation so you pass the raw escapeshellarg($var) return value (which
already includes quotes) into the command string for all four occurrences.
- Line 502: The exec call in CreateDocker.php concatenates $xml['Name'] directly
into the shell command causing a command-injection risk; sanitize/escape the
container name before invoking exec by replacing the concatenation with a safely
quoted argument (e.g., use PHP's escapeshellarg on $xml['Name']) or avoid a
shell invocation entirely (use a direct API or proc_open with argument arrays)
so the exec("docker exec -i ... /bin/sh -c \"tailscale exit-node list\"") call
no longer passes untrusted input to the shell.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec03d287-c535-494c-9804-a3ff03f01847
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpemhttp/plugins/dynamix.docker.manager/include/DockerClient.php
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.docker.manager/include/DockerClient.php
🛑 Comments failed to post (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (2)
213-213:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove extra quotes around
escapeshellarg()results.
escapeshellarg()already adds single quotes around the value. Wrapping the result with additional single quotes produces doubled quotes (e.g.,''value''), which creates invalid shell syntax.For example, line 213:
"--name '" . escapeshellarg($Name) . "'"If
$Name = "test",escapeshellarg($Name)returns'test', so the full string becomes--name ''test'', which is incorrect.🔒 Proposed fix for all four occurrences
Line 213:
-exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker create --name '" . escapeshellarg($Name) . "' '" . escapeshellarg($Repository) . "'"); +exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker create --name " . escapeshellarg($Name) . " " . escapeshellarg($Repository));Line 221:
-exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker rm '" . escapeshellarg($Name) . "'"); +exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker rm " . escapeshellarg($Name));Line 292:
-exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker create --name '" . escapeshellarg($Name) . "' '" . escapeshellarg($Repository) . "'"); +exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker create --name " . escapeshellarg($Name) . " " . escapeshellarg($Repository));Line 300:
-exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker rm '" . escapeshellarg($Name) . "'"); +exec("/usr/local/emhttp/plugins/dynamix.docker.manager/scripts/docker rm " . escapeshellarg($Name));Also applies to: 221-221, 292-292, 300-300
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 213-213: Dynamic command passed to a shell execution function. Use escapeshellarg() and escapeshellcmd() to sanitize input, or avoid shell execution entirely.
(coderabbit.command-injection.php-shell-exec)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php` at line 213, The exec() calls in CreateDocker.php construct shell arguments by concatenating extra single quotes around escapeshellarg() results, causing doubled quotes like ''value''; locate the exec(...) invocations that build the docker command (the ones using --name and the repository/tag variables, e.g., where $Name and $Repository/$Tag are passed) and remove the outer single-quote concatenation so you pass the raw escapeshellarg($var) return value (which already includes quotes) into the command string for all four occurrences.
502-502:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCommand injection vulnerability: escape
$xml['Name']before passing toexec().The container name
$xml['Name']is concatenated directly into the shell command without escaping. If a container name contains shell metacharacters (e.g., backticks, semicolons, pipe characters), arbitrary commands could be executed.🔒 Proposed fix
-exec("docker exec -i ".$xml['Name']." /bin/sh -c \"tailscale exit-node list\"", $ts_exit_node_list, $retval); +exec("docker exec -i ".escapeshellarg($xml['Name'])." /bin/sh -c \"tailscale exit-node list\"", $ts_exit_node_list, $retval);🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 502-502: Dynamic command passed to a shell execution function. Use escapeshellarg() and escapeshellcmd() to sanitize input, or avoid shell execution entirely.
(coderabbit.command-injection.php-shell-exec)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php` at line 502, The exec call in CreateDocker.php concatenates $xml['Name'] directly into the shell command causing a command-injection risk; sanitize/escape the container name before invoking exec by replacing the concatenation with a safely quoted argument (e.g., use PHP's escapeshellarg on $xml['Name']) or avoid a shell invocation entirely (use a direct API or proc_open with argument arrays) so the exec("docker exec -i ... /bin/sh -c \"tailscale exit-node list\"") call no longer passes untrusted input to the shell.
- Purpose: update the Dockerman icon cache patch so label-driven icons refresh when the label URL changes. - Before: the 7.3 Docker tab cache guard could keep serving an existing cached icon even after a third-party container changed its net.unraid.docker.icon label. - Problem: that preserved the fast path but meant the rebased PR no longer fully fixed the original stale icon case. - New behavior: non-Dockerman containers track the last label icon URL, refresh icons when that URL changes, and clear cached label icons when the label is removed. - Implementation: keep Dockerman-managed containers on template-first icon resolution, prefer label URLs only for non-Dockerman containers, share persistent icons by SHA1 URL filename, and purge unused cached icon files.
elibosley
force-pushed
the
dockerman_icon_patch
branch
from
2f8ebfa to
d6fa31c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
488-515: 📐 Maintainability & Code Quality | 💤 Low valueVerify persistent icon glob pattern matches intended files
The second cleanup loop (line 499) uses glob pattern
{images}/{contName}*.png, but the new persistent icon format isicon-<sha1>.png(no container name prefix). This loop will only match OLD-format persistent icons that had the container name in the filename.If this is intentional migration cleanup, the comment should clarify that. If this should also clean new-format icons, the pattern needs adjustment — but the third loop (lines 506-514) handles orphaned
icon-<sha1>.pngfiles, so the current logic may be correct.Also, the condition on lines 494 and 501 uses a confusing double-negative:
if ( ($keepIcon === '') || !(strpos($filename, $keepIcon) !== false) )Suggested simplification
- if ( ($keepIcon === '') || !(strpos($filename, $keepIcon) !== false) ) { + if ($keepIcon === '' || strpos($filename, $keepIcon) === false) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@emhttp/plugins/dynamix.docker.manager/include/DockerClient.php` around lines 488 - 515, The purgeUnusedIconFiles function currently uses a persistent-icon glob that only matches OLD-format filenames (sprintf('%s/%s*.png', $dockerManPaths['images'], $contName)) and also uses a confusing double-negative condition (if ( ($keepIcon === '') || !(strpos($filename, $keepIcon) !== false) ) ). Either (A) if the intent is to only remove old-format persistent icons, add a clarifying comment above the second glob explaining this migration/intent and simplify the condition to if ($keepIcon === '' || strpos($filename, $keepIcon) === false) { `@unlink`($filename); }, or (B) if you need to also delete new-format persistent icons (icon-<sha1>.png), change the second glob to match the new format (e.g. sprintf('%s/icon-*.png', $dockerManPaths['images'])) and likewise simplify the condition as above; apply these changes in purgeUnusedIconFiles to replace the double-negative check and either add the comment or update the glob pattern accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix.docker.manager/include/DockerClient.php`:
- Around line 340-359: DockerClient::getAllInfo() currently calls
getIcon($image, $name, $labelIconUrl ?: ($tmp['icon'] ?? ''), $preferLabelIcon)
which can pass a local filesystem path (from $tmp['icon']) into getIcon() as
$imgUrl so download_url() tries to fetch it and fails; change the third argument
so we never forward a local file path — pass $labelIconUrl if non-empty,
otherwise pass an empty string (e.g. $labelIconUrl ?: '') when calling
getIcon(), leaving local $tmp['icon'] untouched; ensure $preferLabelIcon logic
and the subsequent assignment of $tmp['icon-url'] and fallback question.png
behavior remain the same.
---
Nitpick comments:
In `@emhttp/plugins/dynamix.docker.manager/include/DockerClient.php`:
- Around line 488-515: The purgeUnusedIconFiles function currently uses a
persistent-icon glob that only matches OLD-format filenames
(sprintf('%s/%s*.png', $dockerManPaths['images'], $contName)) and also uses a
confusing double-negative condition (if ( ($keepIcon === '') ||
!(strpos($filename, $keepIcon) !== false) ) ). Either (A) if the intent is to
only remove old-format persistent icons, add a clarifying comment above the
second glob explaining this migration/intent and simplify the condition to if
($keepIcon === '' || strpos($filename, $keepIcon) === false) {
`@unlink`($filename); }, or (B) if you need to also delete new-format persistent
icons (icon-<sha1>.png), change the second glob to match the new format (e.g.
sprintf('%s/icon-*.png', $dockerManPaths['images'])) and likewise simplify the
condition as above; apply these changes in purgeUnusedIconFiles to replace the
double-negative check and either add the comment or update the glob pattern
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b1814987-5e6b-40fc-badb-0055a7fbd8fc
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpemhttp/plugins/dynamix.docker.manager/include/DockerClient.php
| $isDockerman = ($ct['Manager'] ?? '') === 'dockerman'; | ||
| $preferLabelIcon = !empty($labelIconUrl) && !$isDockerman; | ||
| $labelIconChanged = $preferLabelIcon | ||
| && ($tmp['icon-url'] ?? '') !== $labelIconUrl; | ||
| $labelIconRemoved = !$isDockerman && !empty($tmp['icon-url']) && empty($labelIconUrl); | ||
| $iconExists = !empty($tmp['icon']) | ||
| && (is_file($tmp['icon']) || is_file($docroot . $tmp['icon'])); | ||
| if (!$iconExists || $reload) { | ||
| $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon'] ?? '')); | ||
| if ($labelIconRemoved) { | ||
| $this->purgeUnusedIconFiles($name); | ||
| $tmp['icon'] = '/plugins/dynamix.docker.manager/images/question.png'; | ||
| unset($tmp['icon-url']); | ||
| } elseif (!$iconExists || $reload || $labelIconChanged) { | ||
| $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon'] ?? ''), $preferLabelIcon); | ||
| if ($preferLabelIcon) $tmp['icon-url'] = $labelIconUrl; | ||
| // Explicitly return the fallback asset, so that subsequent polls see | ||
| // the local file instead of rerunning the expensive template scan | ||
| if (empty($tmp['icon'])) $tmp['icon'] = '/plugins/dynamix.docker.manager/images/question.png'; | ||
| } elseif (!$preferLabelIcon) { | ||
| unset($tmp['icon-url']); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how $tmp['icon'] is used elsewhere to understand the expected value type
rg -n -C3 "\\\$tmp\['icon'\]" --type=phpRepository: unraid/webgui
Length of output: 2279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="emhttp/plugins/dynamix.docker.manager/include/DockerClient.php"
# Show getIcon signature + body
rg -n "function\s+getIcon\s*\(" -n "$FILE" && \
awk 'NR>=440 && NR<=520 {print NR ":" $0}' "$FILE"
# Find all getIcon calls and context
rg -n "->getIcon\(" "$FILE" && \
rg -n -C3 "->getIcon\(" "$FILE"
# Inspect purgeUnusedIconFiles for the glob/pattern logic mentioned
rg -n "function\s+purgeUnusedIconFiles\s*\(" "$FILE" && \
awk 'NR>=480 && NR<=540 {print NR ":" $0}' "$FILE"Repository: unraid/webgui
Length of output: 5516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="emhttp/plugins/dynamix.docker.manager/include/DockerClient.php"
# Inspect download_url implementation
rg -n "function\s+download_url\s*\(" "$FILE" && \
awk 'NR>=520 && NR<=620 {print NR ":" $0}' "$FILE"
# Find where $tmp['icon-url'] is assigned/used in this function/file
rg -n "\$tmp\['icon-url'\]" "$FILE" -S && \
rg -n -C3 "\$tmp\['icon-url'\]" "$FILE" -S
# Show getAllInfo context around the snippet lines (330-370)
awk 'NR>=320 && NR<=370 {print NR ":" $0}' "$FILE"Repository: unraid/webgui
Length of output: 7440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="emhttp/plugins/dynamix.docker.manager/include/DockerClient.php"
# Show all assignments/usages of tmp['icon'] in this file
rg -n "\$tmp\['icon'\]" "$FILE"
# Show all assignments/usages of tmp['icon-url'] in this file
rg -n "\$tmp\['icon-url'\]" "$FILE"
# Locate any place where $tmp is persisted (webui-info) and might already contain icon/icon-url
rg -n "webui-info|DockerUtil::saveJSON|DockerUtil::loadJSON" "$FILE"
# Inspect how icons are stored/updated around getAllInfo
rg -n "getAllInfo|getAllInfo\(" "$FILE" && \
awk 'NR>=300 && NR<=420 {print NR ":" $0}' "$FILE"Repository: unraid/webgui
Length of output: 39
🏁 Script executed:
#!/bin/bash
set +e
FILE="emhttp/plugins/dynamix.docker.manager/include/DockerClient.php"
echo "== tmp['icon'] occurrences =="
rg -n "\$tmp\['icon'\]" "$FILE" || true
echo "== tmp['icon-url'] occurrences =="
rg -n "\$tmp\['icon-url'\]" "$FILE" || true
echo "== webui-info persistence =="
rg -n "webui-info|DockerUtil::saveJSON|DockerUtil::loadJSON" "$FILE" || true
echo "== getAllInfo region =="
rg -n "getAllInfo|getAllInfo\(" "$FILE" || true
awk 'NR>=260 && NR<=390 {print NR ":" $0}' "$FILE"Repository: unraid/webgui
Length of output: 7676
Fix icon fallback: don’t pass $tmp['icon'] (local file path) into getIcon() as $iconUrl
DockerClient::getAllInfo() calls getIcon(..., $labelIconUrl ?: ($tmp['icon'] ?? ''), ...). When Icon label URL is empty and the template icon is also empty, getIcon() uses that 3rd argument as $imgUrl and passes it to download_url(). If it’s a filesystem path (from $tmp['icon']), the download will fail and icon resolution will fall back to the question image.
Suggested fix
- $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon'] ?? ''), $preferLabelIcon);
+ $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon-url'] ?? ''), $preferLabelIcon);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $isDockerman = ($ct['Manager'] ?? '') === 'dockerman'; | |
| $preferLabelIcon = !empty($labelIconUrl) && !$isDockerman; | |
| $labelIconChanged = $preferLabelIcon | |
| && ($tmp['icon-url'] ?? '') !== $labelIconUrl; | |
| $labelIconRemoved = !$isDockerman && !empty($tmp['icon-url']) && empty($labelIconUrl); | |
| $iconExists = !empty($tmp['icon']) | |
| && (is_file($tmp['icon']) || is_file($docroot . $tmp['icon'])); | |
| if (!$iconExists || $reload) { | |
| $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon'] ?? '')); | |
| if ($labelIconRemoved) { | |
| $this->purgeUnusedIconFiles($name); | |
| $tmp['icon'] = '/plugins/dynamix.docker.manager/images/question.png'; | |
| unset($tmp['icon-url']); | |
| } elseif (!$iconExists || $reload || $labelIconChanged) { | |
| $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon'] ?? ''), $preferLabelIcon); | |
| if ($preferLabelIcon) $tmp['icon-url'] = $labelIconUrl; | |
| // Explicitly return the fallback asset, so that subsequent polls see | |
| // the local file instead of rerunning the expensive template scan | |
| if (empty($tmp['icon'])) $tmp['icon'] = '/plugins/dynamix.docker.manager/images/question.png'; | |
| } elseif (!$preferLabelIcon) { | |
| unset($tmp['icon-url']); | |
| } | |
| $isDockerman = ($ct['Manager'] ?? '') === 'dockerman'; | |
| $preferLabelIcon = !empty($labelIconUrl) && !$isDockerman; | |
| $labelIconChanged = $preferLabelIcon | |
| && ($tmp['icon-url'] ?? '') !== $labelIconUrl; | |
| $labelIconRemoved = !$isDockerman && !empty($tmp['icon-url']) && empty($labelIconUrl); | |
| $iconExists = !empty($tmp['icon']) | |
| && (is_file($tmp['icon']) || is_file($docroot . $tmp['icon'])); | |
| if ($labelIconRemoved) { | |
| $this->purgeUnusedIconFiles($name); | |
| $tmp['icon'] = '/plugins/dynamix.docker.manager/images/question.png'; | |
| unset($tmp['icon-url']); | |
| } elseif (!$iconExists || $reload || $labelIconChanged) { | |
| $tmp['icon'] = $this->getIcon($image, $name, $labelIconUrl ?: ($tmp['icon-url'] ?? ''), $preferLabelIcon); | |
| if ($preferLabelIcon) $tmp['icon-url'] = $labelIconUrl; | |
| // Explicitly return the fallback asset, so that subsequent polls see | |
| // the local file instead of rerunning the expensive template scan | |
| if (empty($tmp['icon'])) $tmp['icon'] = '/plugins/dynamix.docker.manager/images/question.png'; | |
| } elseif (!$preferLabelIcon) { | |
| unset($tmp['icon-url']); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@emhttp/plugins/dynamix.docker.manager/include/DockerClient.php` around lines
340 - 359, DockerClient::getAllInfo() currently calls getIcon($image, $name,
$labelIconUrl ?: ($tmp['icon'] ?? ''), $preferLabelIcon) which can pass a local
filesystem path (from $tmp['icon']) into getIcon() as $imgUrl so download_url()
tries to fetch it and fails; change the third argument so we never forward a
local file path — pass $labelIconUrl if non-empty, otherwise pass an empty
string (e.g. $labelIconUrl ?: '') when calling getIcon(), leaving local
$tmp['icon'] untouched; ensure $preferLabelIcon logic and the subsequent
assignment of $tmp['icon-url'] and fallback question.png behavior remain the
same.
6 participants
In 6.10.1+ the dockerman icon caching mechanism does not work correctly for icons specified by the net.unraid.docker.icon container label on containers without a dockerman template. For containers created with this label outside of dockerman the caching mechanism initially downloads and displays the correct icon. If the icon url is changed however the new icon is never downloaded because the old icon remains cached and there is no mechanism in place for invalidating the icon in the absence of a dockerman template. To fix this i propose the following changes to dockerman.
Revision update on June 12, 2026: rebased onto current master at cc6a800 / 7.3.2-rc.0.1 and revised the cache refresh logic for the 7.3 Docker tab fast path. Dockerman-managed containers keep template-first icon resolution. Non-Dockerman containers with net.unraid.docker.icon now track the last label URL, refresh when that URL changes, clear cached label icons when the label is removed, and avoid repeated download attempts for unchanged bad URLs.
Summary by CodeRabbit
Bug Fixes
Refactor