Ver/1.1.2 - #46
Merged
Merged
Ver/1.1.2#46
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
Keep the icon-block migration fix only; the release number and changelog will be added later. Co-authored-by: Cursor <cursoragent@cursor.com>
fix: resolve missing collection during icon-block migration
Co-authored-by: Cursor <cursoragent@cursor.com>
![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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff1ef97. Configure here.
| foreach ( $ordered as $collection ) { | ||
| if ( $collection->get( $name ) ) { | ||
| return $collection->name(); | ||
| } |
There was a problem hiding this comment.
Migration crashes on failed collections
Medium Severity
The new collection lookup calls get() on every value from get_icon_collections() without checking the type. Failed folder or sprite registrations store false in that list, so a WP-CLI migration that hits one fatals instead of using the name/collection fallback this release added.
Reviewed by Cursor Bugbot for commit ff1ef97. Configure here.
| # Run a command inside the CLI container, as the web user, at the WordPress root. | ||
| incli() { | ||
| docker exec -u 33 -w /var/www/html "$PERF_CLI_CONTAINER" "$@" | ||
| } |
There was a problem hiding this comment.
Perf harness hardcodes container UID
Medium Severity
incli() runs docker exec -u 33 against the wp-env CLI container. wp-env owns wp-content as the host user, not www-data, so media import, cache flush, and log collection fail on Linux. tests/bin/phpunit.sh already resolves the UID at runtime with id -u.
Triggered by learned rule: wp-env PHPUnit runner: inherit container env, never hardcode UID
Reviewed by Cursor Bugbot for commit ff1ef97. Configure here.
petitphp
approved these changes
Sep 17, 2026
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
4 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
Touches runtime icon loading, object-cache behavior, and migration output on every request; mitigated by a large integration suite and CI, but regressions could affect memory use, cache warming, or migrated content.
Overview
Release 1.1.2 refactors how icon collections are built and cached so front-end requests no longer load every SVG into memory, and adds first-class support for media-library icons plus broader test coverage in CI.
Performance and caching: Collections now register a lightweight index; SVG bytes load on demand via
ContentLoaderandCollectionItem::from_source(). Folder/file sources defer reads; cache keys are format-versioned (v3) and salted with plugin version. Payloads cache per icon;Cacheskips object-cache writes over ~900 KB (filterblockparty_icons_cache_max_item_bytes) to avoid memcached’s silent 1 MB reject loop.New API:
register_icon_collection()/add_icons()supporttype => attachmentswith optionalqueryforWP_Query; theme example switchesmediathequefrom a manual attachment loop to this type.Migration:
IconBlockMigratorno longer skips blocks with a name but no collection—it resolves collection from registered collections (preferred order) then falls back tomediatheque/icon-pack.Tooling: PHPUnit integration suite (
tests/phpunit,phpunit.xml.dist),npm run test:php, GitHub Actions PHP 8.1–8.4 matrix with wp-env; reproducible benchmark harness undertests/perf.Reviewed by Cursor Bugbot for commit ff1ef97. Bugbot is set up for automated code reviews on this repo. Configure here.