Skip to content

feat: version the editor node cache file names - #339

Draft
somethingwithproof wants to merge 2 commits into
developfrom
feat/versioned-cache-names
Draft

somethingwithproof wants to merge 2 commits into
developfrom
feat/versioned-cache-names

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Fixes #338

Build WeatherMapNode::update_cache() file names in one helper (lib/node-cache.php), keyed by WM_NODE_CACHE_VERSION.

  • Version 1 is the existing node_<md5>.png name, byte for byte, so nothing is renamed. Later formats would use node_v<N>_<hex>.png, which cannot collide with version 1.
  • On its first write to a directory in each process, the writer removes regular files that use another version's name. It does not follow symlinks or recurse.
  • If md5() is unavailable (FIPS), the name uses a 32-character sha256 prefix instead.
  • Note: nothing on develop calls update_cache(), so the node cache is not written today. This PR only prepares the name for a future editor caller or format change. If the method should be removed instead, I can close this.
  • Verified: the full Pest suite passes (380), and patch coverage is 100% (30/30). Unit tests cover the exact version 1 names, the FIPS fallback and every cleanup case. An E2E test renders a real map and calls the real writer.

Comment thread lib/node-cache.php Fixed
Comment thread tests/E2E/NodeCacheE2ETest.php Fixed
@alcatron

alcatron commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Tested 058a257: 381 PHP tests passed. No new failure in the browser checks beyond the existing hover-test fixture issue fixed by #333.

My main question is the need for this now. As you noted in the description, update_cache() has no production callers on develop. Is there a planned caller that needs the versioned names and pruning? If not, I'd lean towards deferring this until the cache is used again. I didn't find a failing test here; this is more about whether the extra code is worth maintaining at the moment.

somethingwithproof added a commit that referenced this pull request Nov 9, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

#339 is held back until the node cache has a production caller again.
@somethingwithproof
somethingwithproof marked this pull request as draft November 9, 2026 19:32
@somethingwithproof

Copy link
Copy Markdown
Member Author

Agreed, there's no production caller for update_cache() on develop right now, so this isn't worth carrying yet. I've moved it to draft and taken its entry out of the batch CHANGELOG in #333. I'll pick it up again if the node cache gets used.

TheWitness pushed a commit that referenced this pull request Nov 11, 2026
* test(browser): keep image-map hotspots focusable under jsdom 30.1.2+

jsdom 30.1.2 started refusing focus to any element with a display:none
ancestor, and its UA stylesheet gives area display:none. Browsers still
focus image-map hotspots, so area.focus() became a silent no-op in the
test: the popup never opened on focus, and the later ArrowDown opened a
fresh popup whose link the stale tip reference did not hold.

The viewer is unchanged; the failure reproduces with Cacti 1.2.x and
develop jQuery UI alike and disappears on jsdom 30.1.1. Render area
inline in the fixture and assert up front that the hotspot takes focus
and opens its popup, so a future jsdom change fails with that message.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* ci: run the jsdom browser tests with locked dependencies

Add a private package.json and lockfile for tests/Browser pinning jsdom
30.1.3 and jQuery 3.7.1, and a dependency-free runner that executes each
*.cjs check in its own process, names each file and stops at the first
failure. It also fails when CACTI_JS_DIR lacks jquery-ui.js or no tests
are found.

A separate browser-tests job installs Node 24, runs npm ci with install
scripts disabled and runs the suite against the Cacti core checkout's
include/js. It needs no PHP or database, so it runs once rather than on
every integration matrix leg.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* docs: record the editor and engine maintenance batch in the changelog

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: require the Node.js releases the locked jsdom supports

jsdom 30.1.3 supports Node.js 24.15 or later on the 24 line and 26 and
later; package.json allowed any Node.js 24.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* docs(changelog): drop the deferred node cache entry

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

#339 is held back until the node cache has a production caller again.

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Build the node cache name in one helper, lib/node-cache.php, keyed by an
explicit WM_NODE_CACHE_VERSION. Version 1 is the existing
node_<md5>.png name, so current files keep byte-identical names.
Later formats use node_v<N>_<hex>.png.

WeatherMapNode::update_cache() now uses the helper. On its first write
to a cache directory in a process, it removes node cache files from
other format versions. Only regular files that match the plugin's own
name patterns are removed. Symlinks, subdirectories and unrelated files
are left in place. When md5() is unavailable (some FIPS builds), the
name falls back to a 32-hex sha256 prefix.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Version 1 cache file names are MD5 of the map and node names by
contract; the hash names a file and protects nothing.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
somethingwithproof force-pushed the feat/versioned-cache-names branch from 058a257 to 9cfdf74 Compare November 11, 2026 22:59
@sonarqubecloud

Copy link
Copy Markdown

@somethingwithproof

Copy link
Copy Markdown
Member Author

Status: parked as a draft on purpose. Nothing on develop calls WeatherMapNode::update_cache(), so the node cache is never written, and versioned file names have nothing to protect yet. As discussed above, I'll mark this ready for review again when a caller for the node cache lands or the cache file format needs to change. Until then it stays out of the review queue and the CHANGELOG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Version the node cache file names

3 participants