Fix 92477 - #45
Closed
stephane-gillot wants to merge 16 commits into
Closed
Fix 92477#45stephane-gillot wants to merge 16 commits into
stephane-gillot wants to merge 16 commits into
Conversation
Release 1.1.1
Legacy blocks with an icon name but no collection were skipped entirely. Look up the name in registered collections, then fall back to mediatheque or icon-pack, and bump to 1.1.2. Co-authored-by: Cursor <cursoragent@cursor.com>
The plugin had no tests. This adds 176 of them, describing what the plugin does today so that a later refactor of how icons load and cache can be shown not to change it. Covered: - every SVG source — folder, sprite, single file — including icon_map labels, sprite version cache-busting, the public URL a sprite icon resolves to, empty and unreadable inputs, and the blockparty_icons_svg_parse_tags filter - the collection container: lookup by name, the dotted-name key, replacement, count, search across both name and label - the registration API a theme calls, duplicate rejection, unknown types, and the editor's REST preload paths - front-end markup: raw and sprite icons, size, border radius, link and aria-label, all accepted and rejected colour formats, and URL/label escaping - both REST controllers: payload shape, pagination headers, search, single item, 404s, and the permission checks at every role - the object-cache wrapper, including that an empty result is a hit and not a miss - the KSES allowances that keep saved block markup valid, and that <script> inside an SVG is still stripped They are characterization tests: they assert observable behaviour — how many icons, with what names, labels, types and contents, and what markup reaches a page — and say nothing about when or how often files are read. A change that alters the mechanism leaves them green; a change that alters what a caller sees does not. Infrastructure: integration tests against a real WordPress rather than mocks, since the plugin leans on WP_Query, the object cache, WP_HTML_Tag_Processor, WP_REST_Controller and a set of filters. The bootstrap runs both through wp-env (npm run test:php) and against the Composer copy of WordPress (composer test), which is the path CI takes on PHP 8.1 through 8.4. Two findings recorded along the way, both in tests/README.md: Collection's Iterator never yields anything because it walks integer positions over name-keyed items, and the development-mode cache bypass cannot be exercised from a plugin suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All four PHPUnit jobs failed on their first run: 23 failures, every one a rendering test receiving an empty string, plus the assertion that the block type is registered. The cause was not the tests. The plugin calls register_block_type() on its build/ directory, build/ is git-ignored, and the workflow never compiled it — so the block never registered and render_block() had nothing to return. Confirmed locally by moving build/ aside: the same 23 failures over the same 408 assertions as CI. The workflow now builds the assets once in its own job and hands them to the matrix through an artifact, rather than running npm four times over, and checks build/block.json exists before handing over to PHPUnit. The bootstrap gained the same check with a pointed message, because a fresh clone lands in exactly this state and two dozen unexplained failures are a poor welcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The workflow was standing up its own MariaDB service and generating a wp-tests-config.php, duplicating what wp-env already provides — and it is where the first CI failure came from. wp-env supplies WordPress, the WordPress PHPUnit library and the database, so the workflow now runs `npm run test:php`, the same command as locally. A green run on a laptop and a green run on the pull request now mean the same thing. The PHP matrix is kept: wp-env takes the version from WP_ENV_PHP_VERSION and pulls the matching upstream wordpress:php8.x image. Documented in tests/README.md, since it is also how you reproduce a single matrix job locally. The hand-rolled path stays supported for anyone without Docker — the bootstrap still falls back to the Composer copy of WordPress and tests/wp-tests-config-sample.php is still there — it just is not what CI uses any more. On failure the job prints the wp-env test-environment logs, which is the first thing anyone would ask for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
86 of 176 tests failed in CI on every PHP version, all with the same message: the fixture directory under wp-content could not be created. The runner script was doing `docker exec -u 33`. wp-env builds its containers around the *host* user — wp-content is owned by that UID and Apache runs as it — so 33 is only right by accident. On macOS it works because Docker Desktop's file sharing ignores ownership; on a Linux runner, where the host UID is 1001 and ownership is real, www-data cannot write there. The same mismatch was behind the PHPUnit result-cache permission warning. Resolving the UID at run time matches what wp-env actually configures, in both places. Verified locally on PHP 8.1, 8.2, 8.3 and 8.4 through wp-env: 176 tests, 427 assertions, green on each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review flagged the runner as never setting WP_TESTS_DIR, and concluded the suite must be falling back to the Composer copy of WordPress and a root wp-tests-config.php. It is not: wp-env sets the variable on its containers and docker exec inherits it, so the bootstrap takes its managed-environment branch and uses wp-env's WordPress, test library and database. The reading was wrong but the confusion was fair — the script leaned on a container-level variable it never mentioned. Both ends of that coupling now say so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test: characterization suite for every feature and SVG source
Stands up an isolated WordPress instance (port 8899, so it runs alongside the normal dev env) holding 500 SVG icons: 200 in a theme folder and 300 contributed through the media library, sized from ~11 KB to ~1.95 MB with 25 above 1 MB. The rig exists to produce comparable before/after numbers rather than assertions. It reproduces the three things that make WordPress VIP hurt: - a persistent object cache that refuses items over 1 MB the way memcached does, silently returning false, which is exactly what no caller checks; - media-library reads routed through a bpifs:// stream wrapper, so they are counted exactly and can be charged VIP Files-like latency; - collections registered on every request, front end included. Metrics land as one JSON record per request and are reduced to medians. `resident` -- the SVG bytes actually held in memory -- is read by reflecting on CollectionItem's private properties rather than through content(), so the measurement stays honest once loading becomes lazy. The object cache is file-backed rather than memcached: it reproduces the semantics exactly but not the latency, so operation counts are the transferable metric and wall time only indicative. That and the other limits are written up in tests/perf/README.md. phpcs now excludes ./tests/: grumphp passes changed files to phpcs explicitly, which overrides the ruleset's file list, and the harness has to break WordPress conventions to do its job -- a cache drop-in must assign $GLOBALS['wp_object_cache'] and declare both a class and the wp_cache_* functions in one file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With 500 icons totalling ~102 MB, every request held all of it in memory -- including a front-end page containing no icon block at all. At PHP's common 128 MB limit the plugin did not merely run slowly, it fatalled while serializing a collection for the cache. Three changes, measured with tests/perf against 500 icons on a cache that refuses items over 1 MB: 1. Registering a collection now builds a lightweight index. CollectionItem holds a small serializable descriptor saying where its bytes live, and ContentLoader reads them the first time content() is called -- one icon to render a block, one page's worth to list a collection in the editor. __serialize() keeps resolved payloads out of the cached index so it cannot grow with use. 2. Payloads are cached individually, and Cache::set_cache() declines anything over 900 KB. Memcached (VIP and most managed hosts) refuses items above 1 MB and signals it only through a return value nothing checks, so an oversized entry was rebuilt, refused and rebuilt again forever. Sixteen entries were in that state in the baseline, including the whole 200-icon folder collection as a single entry, which meant re-reading all 200 files on every request. The ceiling is adjustable with the blockparty_icons_cache_max_item_bytes filter. 3. New `attachments` collection type for SVGs contributed through the media library: one query and one cached index, salted on the posts cache's last_changed so a newly uploaded icon appears at once. It replaces the pattern the README used to recommend -- a WP_Query plus a from_file() call per attachment -- which cost a query and a cache round trip per icon per request. Cached index keys carry a format revision. A salt cannot do that job: wp_cache_get_salted() unserializes the stored value before it compares salts, so an entry written by an earlier release would be unserialized into the new class shape before anything could reject it. Typed properties also gained defaults so such an entry cannot leave one uninitialised. Measured on 500 icons (median of 4 runs), front-end page with no icon block: resident SVG 101.7 MB -> 0.015 MB, object-cache gets 301 -> 2, refused writes 16 -> 0, blockparty_icons_init 602 ms -> 2.3 ms. Charging 2 ms per media read to emulate VIP Files, the same page on a cold cache goes 4806 ms -> 422 ms. Not addressed here: the editor still bootstraps ~22 MB because block_editor_rest_api_preload_paths inlines the first 50 icons of each collection with their content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous comment read as though it were fixing something the code does today. It is not: the factories cache their index before anything calls content(), so no resolved payload can currently reach the cache. It is a structural guard, and the comment now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
They guarded against a resolved payload reaching a cached index, but no call path does that: every factory caches its index before anything calls content(). Default PHP serialization is enough, and two fewer magic methods is two fewer things to reason about. Cached indexes stay comfortably cacheable without them -- 108 KB for the 200-icon folder collection, 138 KB for 300 media attachments, against a 900 KB ceiling. Measured metrics are unchanged: resident SVG, object-cache operations, refused writes and response bytes are all identical, and the timing difference is container jitter (repeat runs of this same code give 1.8-2.3 ms for the init hook, matching the previous commit). CACHE_FORMAT goes to v3 because the on-disk shape changed: an entry written by __serialize() would otherwise be unserialized into public dynamic properties, leaving the real typed ones uninitialised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The exclusion was added for tests/perf, whose object-cache drop-in has to assign $GLOBALS['wp_object_cache'] and declare a class alongside the wp_cache_* functions. Written as ./tests/ it now also covers the PHPUnit suite that landed on develop, which is phpcs-clean and should stay linted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both columns re-taken in one session on the same machine and the same 500 icons, the "before" side by checking the original classes back into the working tree rather than reusing an older run. Every operation count is unchanged — resident SVG 101.7 MB to 0.015 MB, 301 cache gets to 2, 16 refused writes to none. Wall times shifted by a few tens of percent, which is what wall times do; the table now reports what was actually measured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
perf(icons): load SVG payloads on demand, and add a benchmark to prove it
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 01eac1c. Configure here.
| * so an icon larger than the backend's item limit simply stays uncached | ||
| * instead of failing a write on every single request. | ||
| */ | ||
| Cache::set_cache( $cache_key, $content, self::CACHE_GROUP, $salt, DAY_IN_SECONDS ); |
There was a problem hiding this comment.
Stale SVG cache after media updates
Medium Severity
The attachments index is salted with wp_cache_get_last_changed( 'posts' ), so it rebuilds as soon as an attachment is updated, but ContentLoader caches SVG bytes by path and plugin version only. Replacing a media-library file keeps the same attachment id and path, so content() keeps serving the previous payload until that entry expires.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 01eac1c. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Changes core icon loading, caching, and registration paths used on every front-end and REST request; behavior is heavily tested but cache-format bumps and migration fallbacks could affect large sites with legacy content.
Overview
Release 1.1.2 overhauls how icon collections load and cache SVGs, adds a media-library collection type, hardens migration, and ships PHPUnit CI plus a local performance harness.
Icon registration now builds lightweight indexes instead of reading every SVG up front. Folder and single-file icons use deferred
CollectionItem::from_source()descriptors resolved by a newContentLoaderthat caches each payload separately.Cache::set_cache()skips object-cache writes over ~900 KB (tunable viablockparty_icons_cache_max_item_bytes) to avoid memcached’s silent 1 MB reject loop. Index cache keys are versioned (v3prefix + plugin version salt) so older serialized entries are not reused.A new
attachmentscollection type (optionalqueryargs) registers all media-library SVGs from oneWP_Queryand one cached index;register_icon_collection()/add_icons()wire it throughCollection::from_attachments(). The wp-env theme example switchesmediathequeto this API instead of per-attachmentfrom_file()loops.beapi/icon-blockmigration no longer skips icons with a name but no collection: it resolves the name across registered collections (preferred order) then falls back tomediatheque/icon-pack.Tooling: PHPUnit integration suite (~176 tests),
phpunit.xml.dist,test-php.ymlmatrix on PHP 8.1–8.4 (wp-env, npm build, composer),tests/perfbenchmark protocol, and docs/README/MIGRATION/CHANGELOG updates.Reviewed by Cursor Bugbot for commit 01eac1c. Bugbot is set up for automated code reviews on this repo. Configure here.