Implement separate icon and background image libraries

[alcatron]

Replace the image-manager placeholder with separate icon and background uploaders and existing-image libraries. Show previews, dimensions and map references; allow confirmed physical deletion of unreferenced managed images. Accept PNG, JPG/JPEG and GIF (first frame), subject to upload size and pixel limits. Preserve unmanaged current icon selections and accept case-insensitive raster extensions after saving and reloading.

Protect reference scans and deletion with the config-directory mutex. Configuration saves and repairs use checked atomic replacement with revision comparison, so stale data cannot overwrite concurrent edits. Create-only cloning reserves the file before creating map/auth/settings records or invoking the runner, so failed writes leave no broken registrations. Management creation also checks create-only writes. Loaded page revisions reject stale editor submissions. Controls and initial JavaScript metadata share one WeatherMap snapshot; saves reload the full editor state through a clean view URL rather than updating a token beside stale controls. A deletion race producing a null loaded revision returns HTTP 404. INCLUDE reference scanning follows literal renderer paths, including filenames with brackets; the symlink regression uses a temporary library root without renaming shared checkout assets. Clone registration binds both source and destination IDs in prepared statements. Clones retain the mutex through registration and clean their own reservation and partial records on false returns or exceptions. Raw config saves no longer report false post-save conflicts, and request CAS races return HTTP 409. Repair copies legacy assets without removing originals. Initial reference discovery scans .conf maps only, while literal INCLUDE files remain supported. All editor save actions propagate write failures to AJAX callers; stale move/delete targets return errors. The Close control is keyboard-focusable, and upgrade test stubs run in isolated processes.

Snapshot tokens include all loaded INCLUDE dependencies, and their hashes are rechecked at commit time under the appropriate directory mutexes. A newly selected managed image must still exist when the configuration is committed; a deletion-winning race returns HTTP 409 and leaves the configuration unchanged. Existing missing legacy references remain editable for repair. New-map failures retain their save status and show retry guidance, and registration supports readable read-only config directories.

Image validation inspects the proposed serialized ICON/BACKGROUND directives, including raw node overrides. Deletion locks logical and physical configuration sources and rescans references after acquiring those locks, protecting alias/direct writers together.

New map copies validate every inherited managed image reference at commit time. Legacy repair confines sources to the plugin tree and verifies raster decoding. Save and Enter submission restore the active dialog action.

Preserve tokenized icon paths during copy/raw editing, validate concrete inherited references in management cloning before registration, and reject corrupt or symlinked existing repair destinations.

Uploads publish only complete encoded images after flush, close and permission setup; staging files stay outside selectable library names. Create-only copies and management cloning validate effective image references inherited through INCLUDE files, with snapshot dependencies checked under their directory locks. Legacy sized raster icon repair preserves width and height.

Mutation routes require loaded revisions. Clone failures and cleanup errors log internal diagnostics server-side and show generic retry guidance.

New-destination copies bind the loaded source root revision and target identity at commit time. Cleanup attempts run independently and retain the complete config if map-row removal is uncertain; failed unlink is logged. Canonical destination identity preserves CAS across aliases, and logical/physical INCLUDE snapshots detect retargeting.

Validation: affected branch PHP 8.3 and browser regressions pass in isolated Cacti fixtures. The combined changes pass 292 PHP tests (5859 assertions) and 16 browser regression programs. Changed-line coverage passes at 100% of measured production lines; existing controller/entry-point exemptions remain. Syntax, whitespace, translation and manifest checks pass. These revisions have not been deployed to the live installation.

Based on current develop, including the merged icon picker and gallery resize. Shared editor files overlap with the other feature PRs.

[Copilot]

🟡 Changes recommended

Reference detection can permit deletion of templated image dependencies, and required dimensions and usage metadata are not displayed.

9 open findings

• Parse image directives for reliable reference checks · New
• Return image dimensions and reference metadata · New
• Use a larger random suffix to prevent filename collisions · New
• Use translated strings for dynamic dialog text · New
• Reindent the endpoint block with tabs · New
• Localize user-visible exception messages · New
• Add the required GPL v2 header to the support script · New
• Add the required GPL v2 header to the PHP test · New
• Localize new labels and buttons with gettext · New

What changed in this PR

Adds separate node-icon and background-image management libraries to the authenticated editor.

Changes:

• Adds upload, listing, preview, and deletion endpoints.
• Adds responsive image-management UI and styling.
• Adds image-format and deletion regression coverage.

File	Description
weathermap-cacti-plugin-editor.php	Registers image actions and dialog markup.
lib/​editor.assets.php	Implements image validation, storage, listing, and reference checks.
lib/​editor.actions.php	Adds JSON upload/delete request handling.
js/​editor.js	Implements library rendering and AJAX interactions.
css/​editor.css	Styles the responsive image libraries.
tests/​Unit/​EditorImageFormatsTest.php	Runs the image regression fixture.
tests/​Support/​EditorImageFormatsRegression.php	Tests formats, limits, references, and deletion.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Symlinked parent directories can escape deletion safeguards, missing includes disable the feature, and listing scales poorly.

6 open findings

• Reject symlinked images parent directories · New
• Skip unmatched optional configuration includes · New
• Cache configuration reference index per request · New
• Localize new labels and buttons with gettext
• Localize user-visible exception messages
• Use translated strings for dynamic dialog text

6 resolved since last review

• Parse image directives for reliable reference checks
• Use a larger random suffix to prevent filename collisions
• Return image dimensions and reference metadata
• Add the required GPL v2 header to the PHP test
• Add the required GPL v2 header to the support script
• Reindent the endpoint block with tabs

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Uploads can remain persisted after a reference-index failure is reported as an upload failure, and required PHPDoc is missing.

0 open findings

6 resolved since last review

• Reject symlinked images parent directories
• Cache configuration reference index per request
• Skip unmatched optional configuration includes
• Localize new labels and buttons with gettext
• Localize user-visible exception messages
• Use translated strings for dynamic dialog text

Previously missed (3)

In code that hasn't changed since last review

Build reference index before committing uploaded image

lib/​editor.actions.php:1151

Build the reference index before persisting the upload. As written, wm_asset_store_image() commits the file first, then wm_asset_image_list() can throw while scanning an unreadable configuration or when the 1,000-file limit is exceeded. The endpoint then reports a failed upload even though the image remains on disk, and retries create duplicates.

Document new asset functions with complete PHPDoc

lib/​editor.actions.php:1143

Both newly added functions need the repository-required PHPDoc blocks, including descriptions, parameters, and return values.

Add required PHPDoc structure to asset helpers

lib/​editor.assets.php:26

The new asset helpers do not follow the repository-required PHPDoc shape. Add a description, blank separator, @param entries, another blank separator, and @return to each function in this file; the one-line block here also lacks the parameters and return type.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Image deletion can race with configuration writes, and the dimensions display does not satisfy the stated localization requirement.

1 open finding

• Lock configuration references during scan and image deletion · New

Previously missed (2)

In code that hasn't changed since last review

Use a translatable format for image dimensions

js/​editor.js:1679

The dimensions string hard-codes both the ordering and px, so translators cannot localize it, contrary to the PR's stated requirement that dimensions remain localizable. Add a translated dimensions format to wmEditorText, render through that format here, and regenerate cacti.pot.

Add required PHPDoc for the new helper

tests/​Support/​EditorImageFormatsRegression.php:27

This newly introduced helper lacks the PHPDoc block required for every non-vendored function. Document its purpose, parameters, and return value using the repository's standard shape.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Selector refresh and .jpeg reload handling can discard valid existing image selections.

6 open findings

• Image pruning removes unmanaged raster paths · New
• Lock configuration references during scan and image deletion
• JPEG backgrounds are omitted after editor reload · New
• Use cacti_count() instead of raw count() · New
• Add PHPDoc and expand helper control flow · New
• Use cacti_count() in the PHP test · New

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Path portability and selector-refresh defects can corrupt uploads or remove valid selections and deletion safeguards.

9 open findings

• Normalize Windows paths before matching asset references · New
• Match referenced image paths case-insensitively · New
• Image pruning removes unmanaged raster paths
• Lock configuration references during scan and image deletion
• Open GD output streams in binary mode · New
• JPEG backgrounds are omitted after editor reload
• Use cacti_count() in the PHP test
• Add PHPDoc and expand helper control flow
• Use cacti_count() instead of raw count()

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

The extensive filesystem locking, concurrent persistence, deletion, and database-registration changes warrant final human validation.

0 open findings

4 resolved since last review

• Partial image files are published before encoding completes
• Clone validation ignores managed images referenced by includes
• Sized raster icon directives are not parsed or managed
• Create-only copies omit managed images inherited through includes

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Create-only copies can commit stale source data because the source root revision is not checked at commit time.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Create-only copies omit source config from dependency checks

lib/​WeatherMap.class.php:3909

Create-only copies do not include the loaded source root in the commit-time dependency check. When WriteConfig($destination, true) is called by either copy entry point, $sameFile is false, so expectedRevision is null and only INCLUDE hashes are passed here. A source edit or deletion after ReadConfig() but before this write can therefore produce a successful clone of stale source data. Add the source config path and config_revision to the dependency set for new-destination/create-only writes, as map_duplicate() already does explicitly.

🧠 Review effort: Balanced

[alcatron]

Addressed in ac27769. The additional source-snapshot finding is addressed. New-destination/create-only engine writes add the loaded source root path and revision to the commit-time dependency set, including logical symlink identity where applicable. Source directories share the same mutex protocol and snapshot checks as includes. Regressions verify changed and deleted sources cannot produce a destination, while an unchanged source alias copies successfully. Shared alias identity and INCLUDE-retarget safeguards are included too. Validation: 292 PHP tests (5845 assertions), 16 browser regression programs; measured coverage, syntax, translation, manifest and whitespace checks pass. The live installation is unchanged.

[Copilot]

🟡 Changes recommended

Clone cleanup can delete its config while leaving a database registration behind when cleanup queries fail.

2 open findings

• Cleanup failure leaves stale map registration · New
• PHPDoc is attached to the wrong function · New

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Commit-time validation misses normalized aliases of managed image paths, allowing broken references to be committed.

0 open findings

2 resolved since last review

• Cleanup failure leaves stale map registration
• PHPDoc is attached to the wrong function

Previously missed (1)

In code that hasn't changed since last review

Canonicalize directive paths before managed-image detection

lib/​WeatherMap.class.php:2457

Managed-image detection is based on the directive’s raw text, while the deletion scanner normalizes renderer paths. Consequently, renderer-equivalent references such as ./images/objects/foo.png, absolute plugin paths, or Windows separators are protected from deletion when present, but are omitted from commit-time validation here. A raw edit or clone can therefore reintroduce one after the file was deleted and commit a broken reference. Canonicalize each directive path against the plugin root and classify it by containment in the two managed library directories.

🧠 Review effort: Balanced

[bmfmancini]

Reviewed all production hunks plus the new infrastructure. lib/WeatherMap.config.php adds a real directory-mutex (WeathermapConfig\acquire handles unwritable dirs read-only, releases cleanly, dependency locks are non-blocking and sorted to prevent opposing-include deadlock), and atomicWrite does tempnam→full bytes→fflush→fclose→chgrp/chmod→rename with revision CAS (hash_file against sha256 snapshot), INCLUDE-snapshot dependency matching, alias (logical vs physical path) identity checks, and create-only reserves. Uploads are validated by true getimagesize type allowlist (PNG/JPEG/GIF), 5 MB byte and 4 M pixel caps, re-encoded via GD under a random 60-char stem name with 0644 hardlink-reserve-then-unlink staging; managed libraries reject symlinks. Reference scans protect dynamic {token} paths conservatively (regex .* over literal segments) and the deletion path locks the configs directory plus every covered include directory before unlinking, rejecting referenced images. Editor/mgmt mutation paths require loaded-revision tokens (hash_equals) and now return 404/409/500/422 semantics; repair-maps keeps behavior with atomic staged copies and raster validation instead of blind rename/copy. WriteConfig's locale decimal-point bug is fixed (%f→%F plus explicit '.' for bandwidth strings) — configs written under comma-decimal locales were corrupt before. eval() is confined to test-harness files; no eval in production code. I approved the previously action_required workflow runs: Plugin Integration Tests and CodeQL now pass on this head (PHP 8.2/8.3/8.4 integration green). Note for merge order: branch is DIRTY against develop (overlaps #301/#302 shared files) — conflicts must be resolved and re-verified before landing. Approving the changes as reviewed.

[Copilot]

🟡 Changes recommended

Mutation routes lack the promised revision validation, and unsupported image contents can enter selectors.

2 open findings

• Mutation routes bypass loaded-revision validation · New
• Unsupported image formats are added to the library · New

🧠 Review effort: Balanced

[alcatron]

Follow-up audit fixed the initial selector path as well as the AJAX library listing in f07f47a. get_imagelist now rejects non-files, symlinks, unreadable images, invalid image bytes and detected formats outside PNG/JPEG/GIF. Regression tests check that invalid data and BMP contents disguised as PNG are omitted from both the library list and initial selectors, in both icon and background categories.

Validation: 289 PHP tests and all browser regression programs pass. Syntax, whitespace, manifest and translation checks pass. Measured changed production PHP coverage is 100% (648/648 lines), with existing entry-point/controller exemptions.

[Copilot]

🟡 Changes recommended

Path handling and incomplete deletion locks can leave broken image references, while selector filtering can clear existing backgrounds.

2 open findings

• Lock and rescan missing INCLUDE destinations before deletion · New
• Keep readable symlinked backgrounds selectable · New

2 resolved since last review

• Unsupported image formats are added to the library
• Mutation routes bypass loaded-revision validation

Previously missed (2)

In code that hasn't changed since last review

Normalize managed-image paths before existence checks

lib/​WeatherMap.class.php:2459

Both managed-image collectors recognize only literal images/objects/ and images/backgrounds/ prefixes. Renderer-valid paths such as ./images/objects/router.png, images/./objects/router.png, or absolute library paths therefore bypass commit-time existence checks. Raw edits can save a deleted image, and copies or management clones can inherit missing images through INCLUDE files. Normalize path identities consistently in both collectors and the loaded-reference baseline, including missing files, while preserving the original serialized paths and renderer tokens.

Preserve symlink semantics when resolving relative INCLUDE paths

lib/​editor.assets.php:113

Lexically collapsing .. before resolving an INCLUDE changes its meaning when a preceding directory is a symlink. For INCLUDE linked/../include.conf, the renderer reads from the symlink target's parent, but this scan may check a different, nonexistent file and skip it. Images referenced only by that include can then be deleted. Make relative INCLUDE paths absolute without collapsing their components, leaving resolution to the filesystem.

🧠 Review effort: Balanced