Repository navigation
Implement the core re-audit: one pack path, bulk registration, short tag, host safety - #172
Merged
Merged
Conversation
The provider applied memory_limit and max_execution_time while it registered, so every host request ran without its own time limit, and it read the runtime config before the config had merged. RuntimeLimits now applies them around the seed, cache and watch operations only, from the merged config, and restores the host's values after (C-01).
The morph map is flat, so a host or package mapping its own model to the bare alias 'icon' replaced core's entry and stored termables rows loaded that model. A reversible migration moves the rows, and boot refuses a map where the alias resolves to another class (C-02).
The queue injects RuntimeLimits, so a caller handling the job itself gets it from the container. The snippet test now pins ichava-icon.
The terms seeder parsed config.json a second time to find variants. The registry already carries them, so a variant registered from code is now seeded too (C-04).
The model had its own copy of 'read an SVG safely' with no minimum size and no explicit symlink refusal. It now reads through the driver and maps a refusal to the empty string and audit line it used before. 0 of 246,854 pack SVGs are under the minimum or symlinked (C-12).
PackConfigCache owns the config.json cache keys, so the watcher forgets a pack's config without reaching ConfigurationService and, through it, IconCacheService (C-05). ConfigurationService parses through Helpers::loadConfigJson() instead of its own copy (C-04).
A golden test over 31 pack directories, written by the registration code as it is today, so the single registration path that follows can prove it changes nothing.
PackBuilder builds a PackDefinition from a config.json, from code, or both, writing each override into the shape config.json uses, so every existing reader sees it. IconRegistry::register() is the one place that builds the set and its metadata; fromDirectory() now delegates to it and keeps its signature. The golden snapshot over 31 pack directories is unchanged.
RegistrationConfig built ten metadata keys where fromDirectory() built the full set, so a set registered through package() had no kind, theme or licence metadata and no aliases.json. It now builds the same definition and registers it on the same path (C-03). Its fluent API is unchanged.
JsonConfigConstants::config() parsed config.json itself, so an override a pack registered through PackBuilder was invisible to its constants class. PackConfigReader returns the registered definition's config, and the file only before the set registers.
A pack's default attributes lived in its IconComponent subclass, so they reached its own tag and not <x-ichava::icon> or render(), while config.defaults in config.json had no reader. The registry now applies config.defaults.class, .attributes, .stroke_width and .fallback to the set it builds (P-06); PackBuilder writes them too. PackBuilder now reads config.json at build() and replays each fluent call over it, so a broken file fails where it is registered. discover() lists the config-bearing sub-directories of a path, for bulk registration.
registerMany() registers PackDefinitions or PackBuilders as one run: one registrar id, a started event (the factory had no dispatcher), each set through register(), one completed with the counts. A broken set is reported and skipped unless failFast. IchavaRegistrar, behind registerBulkIconSets(), now applies the per-set values it was given (prefix and the rest via PackBuilder::withOptions()), discovers every set when given none, gives its five write-only settings a reader, and logs whether enableLogging() comes before or after configure() (C-10). Core's bundled test set registers through registerMany(), so the bulk path runs on every boot.
custom-icons.sets documented enabled, prefix, default_class,
blade_namespace and blade_component, and the registry read path only.
registerFromConfig() now builds each entry through PackBuilder, so every
key applies; a directory with no config.json registers as app/{key} or
its package key, and a bulk entry registers each set under its path (C-09).
ComponentPrefixes registers the canonical ichava-core prefix always and the short one from components.short_prefix (default ichava, null turns it off), each with the component namespace (C-20). The value is validated against Blade's tag parser at boot. components.enabled false registers no core tag (C-09). Doctor gains component.short_prefix_shadowed, read from Blade's live alias map after boot. The deferral test now pins the configured states.
IconPackServiceProvider keeps its two-method simple path. The optional
pack() hook receives a PackSpec: shape the default set with PackBuilder,
add a set or every set under a directory, add Blade components, or name
the config file. Every pack component now registers its canonical
{vendor}-{package}::{name} tag, and the short {package}-{name} tag while
components.short_prefix is on (C-08), through ComponentPrefixes, so
loadBladeComponent() and the spec share one rule.
19 classes no one extends or mocks are now final. FinalByDefaultTest fails on any open concrete class in src/ that is not listed with the reason it stays open (an extender or a mock seam, measured across ichava/packages), and on a listed class that no longer exists (C-14).
ChecksPackUpdates and DatabaseOperations are bound to the concrete singletons, and every core consumer depends on the contract, so a test or host substitutes them without subclassing (C-06). Both classes stay open until icon-browser's own test double moves to the contract.
BuildIconUsageSyntax, ClearDiscoveryCaches, DiscoverInstalledPackages and SearchIconsInDatabase were invoked through __invoke(), the other two actions through handle(). All six now expose handle() alone; their only callers are in core. The discovery cache generation, a second public static on ClearDiscoveryCaches, is now Support\DiscoveryCacheGeneration (C-13).
It has two named entry points, recursively() and directly(), which makes it a counter rather than an action. Moved verbatim; ActionShapeTest pins one public handle() per action (C-13).
IconRegistry takes IconCacheService, Filesystem and SvgFileCounter in its constructor instead of fetching them, so IconSetBuilder is built from injected parts, and IchavaSessionManager takes IchavaLogger. A guard keeps the cleared files at zero app() calls (C-05).
Icon::processedSvg() fetched four collaborators from the container, the one extraction reason C-11 names. The body moved verbatim into Services\IconSvgReader with them injected; the model's method keeps its signature and forwards.
Support\IconPackServiceProvider is now Providers\IconPackServiceProvider, and Support\ServiceProvider is Providers\PackServiceProvider. Moved verbatim, with no alias at the old names (forward-only); every core reference, fixture and doc follows. ProvidersNamespaceTest fails on any *ServiceProvider.php outside a Providers/ directory or namespace (C-07). The 21 packs and the scaffolder blueprint change their import in their own pull requests.
Every pack carried its own copy of the same tests, about 23,000 lines in 21 repos, and five packs had no stray-text check. Core now ships the rules once: Testing\PackTestCase (the base every pack suite used), Testing\PackConformance (one method per rule, PHPUnit Assert, each asserting how many files it read), and Support\SvgInspector (policyViolations() against SvgPolicy, strayTextNodes()). Each method is shown passing and failing in core (P-03, P-04). symfony/yaml joins require-dev, which the kit invokes.
README's quick start shows registering a set from code; the new docs/tools/pack-registration.md covers the simple path, the pack() hook, PackBuilder and its merge contract, and bulk registration. The Blade page leads with ichava-core and documents components.short_prefix. The custom-icons recipe documents the keys that now apply and the real vendor/package::icon names. Architecture's IchavaRegistrar section, which told readers to override a private method, becomes Bulk registration (C-18). CHANGELOG [Unreleased] records every change.
The database contract marks its reads impure, the pack spec is built once without a nullable read, the test kit narrows what it reads from JSON and YAML and reads the environment through Env, and three IconSetBuilder properties nothing reads keep a reasoned ignore, listed for a decision.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
This was referenced Nov 5, 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
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.
Summary
Implements the 2026-11-05 architecture re-audit of core, in the plan's batch order, one commit per finding. Test first throughout: each new guard was run against the code before its fix and failed.
Batch 1, host safety
Support\RuntimeLimits::scope()appliesruntime.*from the merged config around the seed, cache and watch commands,IchavaSeeder::run()andSeedIconsJob, and restores the previous values after. Red before: a hostmemory_limitof3Gbecame1Gon boot (the shipped2Gdefault never took effect either).Icon::MORPH_ALIASisichava-icon. A reversible migration moves storedtermable_typerows; boot throws if the live morph map resolves the alias to another model. No fallback.Batch 2, one reader, one loader
variant_metadata;ConfigurationServiceparses throughHelpers::loadConfigJson().SvgDriver::load(). Measured first: 0 of 246,854 pack SVGs are underMIN_SVG_FILE_SIZEor symlinked.PackConfigCacheowns the config.json cache keys, so the watcher no longer reachesConfigurationService/IconCacheService.JsonConfigConstants::config()reads the registered definition throughPackConfigReader.Batch 3, Proposal 2:
Data\PackDefinition, the fluentSupport\Pack\PackBuilder,IconRegistry::register();fromDirectory(),package()(C-03, C-15) andcustom-iconsdelegate. A golden snapshot over 31 pack directories (19 estate packs, every fixture) is unchanged.Batch 4, Proposal 3:
registerMany(),PackBuilder::discover(),BulkOptions/BulkResult, one registrar id per run withstarteddispatched;IchavaRegistrar/registerBulkIconSets()apply their values (C-10); core's test set registers through it;custom-iconskeys wired, plusbulk/only(C-09).Batch 5, Proposal 1 and pack tags:
View\ComponentPrefixes,components.short_prefix(defaultichava, null turns it off, validated against Blade's parser) andcomponents.enabled; doctorcomponent.short_prefix_shadowed; packs register<x-{vendor}-{package}::icon>always and<x-{package}-icon>while the short prefix is on (C-08, C-20).IconPackServiceProvider::pack(PackSpec), an optional hook:set,addSet,sets,component,mainComponent,configFile.Batch 6, seams and hygiene:
Contracts\ChecksPackUpdatesandContracts\DatabaseOperations(C-06); 19 classes madefinalwith a guarded exception list (C-14); actions expose onehandle(),CountSvgFilesmoves toSupport\SvgFileCounter(C-13); collaborators injected inIconRegistryandIchavaSessionManager(C-05);Services\IconSvgReaderextracted copy-and-delegate (C-11).Batch 7, provider layout (C-07):
Providers\IconPackServiceProviderandProviders\PackServiceProvider, no alias at the old names. The packs and the scaffolder blueprint follow in their own pull requests, after this merges.Packs re-audit, core half:
config.defaultsapplied at registration so every tag renders the same attributes (P-06); the pack test kitTesting\PackTestCase,Testing\PackConformanceandSupport\SvgInspector(P-03, P-04).Docs: README quick start,
docs/tools/pack-registration.md(simple path, advanced path,PackBuildermerge contract, bulk), the Blade page and the custom-icons recipe; CHANGELOG[Unreleased].Not done here, on purpose: C-16 stays a recorded deviation; C-19 is not planned.
Verification
vendor/bin/pest: 1069 passed, 10 skipped (baseline 982 passed, 10 skipped).vendor/bin/phpstan analyse --debug --memory-limit=2G: no errors.composer pint: passed.