IBX-12358: Introduced Bootstrapper / Hooks for integration tests - #38
Conversation
|
Two companion PRs depend on this branch via a temporary
|
There was a problem hiding this comment.
Pull request overview
Introduces shared integration-test bootstrapping with extensible, priority-ordered setup hooks.
Changes:
- Adds database bootstrap and hook execution infrastructure.
- Implements schema, fixture, and index-purge hooks.
- Registers services through PHP configuration and updates test fixtures.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
composer.json |
Adds OptionsResolver dependency. |
src/bundle/DependencyInjection/IbexaTestCoreExtension.php |
Loads PHP service configuration. |
src/bundle/Resources/config/services.php |
Registers executor and hooks. |
src/bundle/Resources/config/services.yaml |
Removes YAML service importer. |
src/contracts/Bootstrapper/Bootstrapper.php |
Adds kernel and database bootstrap flow. |
src/contracts/Bootstrapper/FixtureHook.php |
Adds fixture-import hook. |
src/contracts/Bootstrapper/HookInterface.php |
Defines the hook contract. |
src/contracts/Bootstrapper/HooksExecutorInterface.php |
Defines executor behavior. |
src/contracts/Bootstrapper/PurgeIndexHook.php |
Adds optional index purge. |
src/contracts/Bootstrapper/SchemaHook.php |
Adds schema-import hook. |
src/contracts/IbexaTestKernel.php |
Registers the test bundle globally. |
src/contracts/Resources/test_data.yaml |
Normalizes anonymous-group capitalization. |
src/lib/Bootstrapper/HooksExecutor.php |
Resolves options and invokes hooks. |
tests/integration/TestKernel.php |
Removes redundant bundle registration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/contracts/Bootstrapper/Bootstrapper.php:147
schema_updateis documented as a boolean but bypasses the strict OptionsResolver validation used by every hook. PHP truthiness means an invalid value such as the common string"false"still runs the schema update instead of failing clearly. Validate this option asboolbefore using it.
if ($options['schema_update'] ?? true) {
src/contracts/Bootstrapper/Bootstrapper.php:35
- The public options contract here conflicts with the PR description: callers are told to use
schemaandfixtures, but the executor only reads service-ID sub-arrays and these hooks defineload_schemaandload_fixtures. A caller following the advertised API will therefore be silently ignored and both imports will run. Please align the implementation and documented API (or explicitly support the flat keys).
* - HookClass::class => [...]: each hook's own options, under a key equal to that hook's own
* service id (which, for the built-in hooks below, is their own FQCN), resolved against whatever
* that hook declares in {@see HookInterface::configureOptions()}. For example:
* - \Ibexa\Contracts\Test\Core\Bootstrapper\SchemaHook::class => ['load_schema' => false]
* - \Ibexa\Contracts\Test\Core\Bootstrapper\FixtureHook::class => ['load_fixtures' => false]
* - \Ibexa\Contracts\Test\Core\Bootstrapper\PurgeIndexHook::class => ['purge_index' => true]
…encies in Test Core
Wires SchemaHook, FixtureHook, and the new PurgeIndexHook into the HooksExecutor via tagged services, and simplifies Bootstrapper to just resolve/boot the kernel, prep the database, and delegate everything else to the hook pipeline. Each hook reads its own option key from the bootstrap options array, so a downstream bundle (e.g. ibexa/migrations) can contribute its own hook without any changes needed here.
Migrations is not a concept ibexa/test-core knows about - this method only existed as a leftover from the pre-Hook PoC, where Bootstrapper called it directly. Its only caller is now ibexa/migrations' own MigrationHook, so it belongs on that package's own test kernel instead (see companion commit in ibexa/migrations).
It was only there to resolve the (now fully-removed) migration-specific class references that used to live inline in Bootstrapper.php. Nothing in this package depends on a proprietary Ibexa package anymore.
EXPOSED_SERVICES_BY_CLASS/_BY_ID picked up an `array` type hint in the original PoC commit, even though this package's composer.json still declares php ^7.4 || ^8.0 support. Typed class constants only exist since PHP 8.3.
GlobFileLoader only tracks matched files as a cache-invalidation resource; it needs a configured LoaderResolver to actually parse them, which this extension never set up. It silently never loaded services.php, meaning none of the Hook/Bootstrapper services (added when services.yaml became services.php) were ever registered in any consuming kernel - caught by actually running the two showcase suites end to end.
Without it, this bundle's extension (and therefore every Hook service) never made it into any consuming kernel's container - the underlying reason the previous commit's loader bug went unnoticed for so long. Simplified TestKernel accordingly: it was manually re-registering this bundle as a workaround for the exact gap now fixed at the source.
HooksExecutor and its interface alias were private and referenced by
nothing inside the container graph (Bootstrapper fetches them from
outside, via the test service container) - Symfony's compiler pruned
them as unused before framework.test's public-everything mechanism
ever got a chance to preserve them. Marked both public explicitly.
Separately, SchemaHook/FixtureHook typed their $kernel argument against
the concrete Ibexa\Contracts\Test\Core\IbexaTestKernel class, which does
not autowire: Symfony only resolves the synthetic "kernel" service by
the interfaces it implements (KernelInterface, HttpKernelInterface, or -
as used here - IbexaTestKernelInterface), never by an intermediate parent
class. Retyped against IbexaTestKernelInterface and wired $kernel
explicitly via service('kernel') to sidestep the ambiguity entirely.
This file was copied from ibexa/core's own Legacy fixture data in 2023 and never touched since; that original copy (and every migrations snapshot test calibrated against it) uses lowercase "Anonymous users". Surfaced by actually running ibexa/migrations' full suite against this package's shared kernel.
Each HookInterface implementation now declares its options through a configureOptions(OptionsResolver $resolver) method (mirroring Symfony\Component\Form\FormTypeInterface), so an unknown key or wrong type throws instead of being silently ignored. The top-level options array passed to Bootstrapper/HooksExecutor is keyed by each hook's own service id (not its FQCN), so two differently-configured instances of the same hook class wouldn't collide on one options slot. HooksExecutor is wired via tagged_iterator() with an explicit index attribute (falling back to service id, since no hook sets that attribute) rather than tagged_locator(): Symfony's ServiceLocatorTagPass ksort()s its service map before building the locator, which silently broke the tag-priority execution order hooks rely on (schema before fixtures). Verified against Symfony's own DI source before landing on this, and confirmed the correct order empirically. Added symfony/options-resolver as an explicit require (was only ever resolved transitively before).
An unregistered "framework.test: true" config makes this fail with an opaque ServiceNotFoundException; point at the actual fix instead.
…path
file_exists('./test.db') never matched the actual configured file: this
repo's own suite uses var/test-bootstrapper.db, and even ibexa/migrations'
suite (var/test.db) wouldn't match since the path was missing the "var/"
prefix. Stale data from a previous run was silently never cleaned up.
Derive the path from DATABASE_URL itself instead, same convention the
non-sqlite branch above it already uses to decide whether to drop the
database.
…Resolver syntax Now that each hook's options live under its own service-id key, the generic schema/fixtures names no longer need to stay collision-safe placeholders — renamed to match the load_schema()/load_fixtures() vocabulary IbexaTestCoreInterface already uses. purge_index was already action-oriented, left unchanged. configureOptions() now uses OptionsResolver::define() instead of setDefaults()/setAllowedTypes() pairs.
…space They're built-in HookInterface implementations other packages may want to reference directly (e.g. to depend on their tag priority or to compose their own bootstrap options), so they belong alongside HookInterface/HooksExecutorInterface/Bootstrapper rather than in lib, which this package's own convention treats as internal implementation. HooksExecutor stays in lib — it's an implementation detail behind HooksExecutorInterface, not something downstream code references by class.
The previous commit renamed these files' paths but the namespace declaration inside them never actually got updated to match — the file move was staged separately from the edit, and the edit didn't get re-added before committing. PHPStan caught what the runtime autoloader didn't (a classmap-based autoloader resolves by class name regardless of namespace/path mismatch, so tests passed locally against the correct working-tree files while the actually-committed content lagged behind).
ContainerInterface::get() is typed object|null regardless of the $invalidBehavior argument, so the return type can't be narrowed without an explicit runtime check. Added instanceof guards for both test.service_container and HooksExecutorInterface lookups, throwing a LogicException on mismatch instead of trusting the type. Added symfony/polyfill-php80 explicitly since get_debug_type() (used in both new guards) is otherwise only transitively available.
…getContainer() A missing HooksExecutorInterface binding (e.g. framework.test not enabled) previously surfaced as an opaque ServiceNotFoundException; now it gets the same clear LogicException the other lookup already gives.
- Read DATABASE_URL from $_ENV first (falling back to getenv()), matching doctrine.php's own source of truth. getenv() alone misses a value set via $_ENV without putenv() (e.g. Symfony Dotenv without usePutenv()), silently skipping stale-database cleanup. - Strip only the DSN's own separating slash instead of every leading slash. The prior ltrim() collapsed an absolute configured path (e.g. sqlite://i@i/%kernel.project_dir%/var/data.db, which parses to a path starting with "//") into an incorrect relative one. Neither is a live bug in ibexa-core/ibexa-migrations today (both set DATABASE_URL via phpunit.xml's <env>, which does call putenv(), and both use a relative single-slash path), but this is meant to be a generic entry point other packages plug into.
2b856a6 to
a541437
Compare
| ->allowedTypes('callable', 'null') | ||
| ->deprecated( | ||
| 'ibexa/test-core', | ||
| '4.6.0', |
There was a problem hiding this comment.
Won't it be a matching patch version after the package is released? I assume it won't be released as 4.6.0
There was a problem hiding this comment.
ibexa/test-core package is actually never released. It's only ever had development branch. @alongosz how does this look for you? For an external developer this package might actually not be usable without releases? 🤔
There was a problem hiding this comment.
ibexa/test-corepackage is actually never released. It's only ever had development branch. @alongosz how does this look for you? For an external developer this package might actually not be usable without releases? 🤔
The only reason it's usable for 3rd party is that this is a public package. Overall given the proved contracts structure that gets hardened, it's probably a good idea to start releasing it. Notice that package tagger doesn't know that something is not released and treats it as usual. It's just not being used that way on a tag, so it went unnoticed.
| { | ||
| public function testConfigureOptionsRejectsUnrecognizedTopLevelKey(): void | ||
| { | ||
| $executor = new HooksExecutor(['some.hook.id' => $this->hookWithOptions(static function (OptionsResolver $resolver): void { |
There was a problem hiding this comment.
Maybe we could split it into multiple lines as it's taking a lot of line space? The same applies to similar fragments in this class
Each built-in hook's fixed tag priority (previously a bare integer literal in services.php) is now a public PRIORITY constant on the hook class itself, and the tag name is HookInterface::TAG instead of a repeated string literal. PurgeSearchIndexHook::PRIORITY is negative (-100), not just lower than the other built-in hooks: Symfony's tagged_iterator() defaults an untagged-with- priority service to priority 0, so a downstream hook that doesn't declare one would still have run before this one purged the index if it stayed at its previous value (100) or anything >= 0.
| } | ||
|
|
||
| /** | ||
| * @param array<string, mixed> $options |
There was a problem hiding this comment.
Actually it's possible since phpstan 2.2 :) https://phpstan.org/blog/phpstan-2-2-unsealed-array-shapes-safer-array-keys#introducing-unsealed-array-shapes
I guess, we could try to use that here.
|
I think we have one more example somewhere but i.e |
Per review feedback (bnowak): the bridge was added for packages migrating off IbexaKernelTestTrait::postLoadFixtures()/ IbexaTestCore::loadFixtures()'s own callback param, but every package migrated in the rollout so far ended up using a dedicated hook instead -- grepped all of them, zero consumers of this option anywhere. Removing outright rather than carrying deprecated dead code through the release; the pattern (a separate HookInterface implementation at a lower priority than FixtureHook's) is documented and already used by PurgeIndexAfterFixturesHook, OrderManagementFixturesHook, and PaymentFixturesHook for exactly this shape of problem. Replaced FixtureHookTest's four option-specific tests (all of them were about this option) with direct coverage of FixtureHook's actual gating logic instead.
Per review feedback (bnowak): PurgeSearchIndexHook and PurgeIndexAfterFixturesHook were byte-identical except class name, docblock, and PRIORITY value. Moved the shared constructor, configureOptions(), OPTION_PURGE_INDEX constant, and __invoke() into an @internal abstract base; each concrete hook now only declares its own PRIORITY constant and docblock. Deliberately kept as two separate concrete classes (not one class registered as two services) -- ~15 already-migrated packages key their bootstrap options by PurgeIndexAfterFixturesHook::class, which is only stable because it's a real class with its own FQCN/service id. This refactor doesn't touch that: each subclass keeps its own identity, consumers are unaffected. Also added PurgeSearchIndexHookTest, which didn't exist before -- PurgeIndexAfterFixturesHook was the only one of the two with direct test coverage.
Per team decision on the review thread (Steveb-p/bnowak): in-memory
SQLite can't participate in the per-test DAMA transaction/rollback most
consumers rely on anyway, so there's no real reason to keep supporting
it as a default.
This also resolves the "does doctrine:database:drop actually delete
SQLite files" investigation from earlier in the thread -- it does (and
always has, since DBAL 2.0.0) for a real file-based connection. The
manual SqliteFilePathResolver+unlink() path this PR carried was never
actually needed for that reason; it existed because "sqlite://:memory:"
resolves to a connection with neither a "path" nor a "dbname" param
(parse_url() drops the path component entirely for that exact DSN
form), which doctrine:database:drop can't act on regardless of flags.
Dropping in-memory support removes the need for the workaround outright.
- doctrine.php: default DATABASE_URL is now a real file
("sqlite://i@i/var/test.db"), matching what every already-migrated
package already sets explicitly.
- DatabasePreparer: deleted the sqlite-vs-not branching entirely,
always calls doctrine:database:drop --force. Kept one small
conditional: "--if-exists" is only added for non-sqlite connections,
since it forces a listDatabases() call the SQLite platform doesn't
implement (throws "not supported") -- SQLite doesn't need the flag
anyway, SqliteSchemaManager::dropDatabase() already no-ops safely for
a file that doesn't exist.
- Deleted SqliteFilePathResolver and its test -- no longer used
anywhere.
Verified end-to-end against a real package (checkout, sqlite-backed),
not just "no exception": ran bootstrap fresh (creates the file cleanly,
no error dropping a nonexistent one), then inserted a marker row
directly into the resulting file and ran bootstrap again -- confirmed
the marker table is gone afterward (the file was genuinely dropped and
recreated, not left untouched) and a real test still passes against the
fresh schema.
The previous commit (f36f2b4) claimed this fix but a git mistake on my part (a multi-path "git add -A" that included an already-deleted path, which aborts the whole add silently) meant only the two file deletions got staged -- DatabasePreparer.php and doctrine.php themselves were never actually committed. CI's PHPStan run caught it immediately (still referencing the deleted SqliteFilePathResolver class). This commit contains the actual source changes described in f36f2b4's message.
Was pinned to ~0.1.x-dev, a stale pre-versioning-scheme constraint that predates this rollout. The temporary dependencies.json override happened to alias its branch to satisfy that constraint, masking the bug. Now that the override is gone (ibexa/test-core#38 merged), this resolved to an unrelated old 0.1.x state of ibexa/test-core with no Bootstrapper classes at all, breaking CI. Every other package in this rollout already requires ~4.6.x-dev.
Description:
Supersedes #30 — same goal (kill the copy-pasted bootstrap logic across packages), cleaner split this time.
Bootstrappernow only does infra: resolve/boot the kernel, create the DB, optionally rundoctrine:schema:update. Everything else — schema import, fixture import, search index purge — moved intoHookInterfaceimplementations taggedibexa.test.bootstrapper.hook, run byHooksExecutorin priority order.SchemaHook/FixtureHookwere actually empty stubs before (the tagging was wired up but nothing implemented them yet); they're real now, each gated by its own option key (schema,fixtures) read off the options arrayBootstrapperforwards untouched. AddedPurgeIndexHook(purge_index, defaultfalse) to finish that bit instead of leaving it as dead inline code.The point of tagging instead of hardcoding: a downstream bundle can contribute its own hook without touching this package at all.
ibexa/migrationsdoes exactly that in its own PR — registers aMigrationHookfrom its own test kernel, and it just gets picked up by the sametagged_iterator.Since opening this PR: added
DefaultSchemaFilesProvider/DefaultFixtureProvider(plain,untagged classes any service can constructor-inject to get the built-in schema/fixture defaults
without going through a Kernel method),
Bootstrapper::OPTION_PREPARE_DATABASE(skips DBcreate/schema-update/hook execution entirely for packages whose tests never touch the database),
and
PurgeIndexAfterFixturesHook— several downstream packages (checkout, product-catalog) eachcarried their own copy of a hook purging the search index between fixture import and migration
execution, since the existing
PurgeSearchIndexHook's fixed priority (100) runs after migrations.Moved into this package as a shared, disabled-by-default hook (priority 800) instead of
duplicating it per package.