Repository navigation
Port Laravel Pennant - #67
Merged
Merged
Conversation
The semantic column rule required jsonb() over json() for every JSON value. That is wrong for values the database never inspects: jsonb and MySQL's JSON type normalize object key order and PostgreSQL's jsonb rejects \u0000, so a value read back no longer matches what was written. Reword the rule so jsonb() is for JSON the database queries, indexes or validates, and text() is for opaque serialized values that are only read back whole. Group the existing UUID and network address guidance in the same sentence.
StackStore::many() and putMany() looped over keys and went through the single-key paths, so a cold batch on a Swoole and Redis stack cost one Redis round trip per key for reads and two writes per key for puts. many() now calls each layer's many() once for the keys still missing. As before, any non-null record ends a key's search and only records with a value are restored into the layers above. restoreRecords() fills the deepest layer first, grouping records by absolute expiration into one putMany() per group, and measures the remaining TTL just before each write because earlier reads and writes can yield. putMany() normalizes the records once and calls each layer's putMany() once through callStores(). A batch write can store some keys before it reports failure, so the new rollbackFailedStore option also compensates the failing layer on a false result. The rollback closure finishes a layer's cleanup after cancellation, but stops at the first ordinary failure rather than paying a timeout per key against an unreachable layer; a saved cancellation is still rethrown. Records whose expiration has passed are no longer written. A worker-array layer above Redis could read a record in the second after its integer expiration while Redis still held it, and a non-positive TTL made the array store keep it forever. putToStore() and putToStoreTagged() now forget the key instead, and the batch restore skips such records. normalizeRecord() replaces the normalization duplicated in putToStore() and putToStoreTagged(), and is shared with the restore path. The tests cover batched reads with partial hits in every layer, TTL grouping, forever records, malformed records, expiry measured as time moves during reads and writes, failure compensation of the failed and completed layers, partial writes before an exception, and cleanup after cancellation. Cross-layer order assertions now use globally()->ordered(): plain ordered() only orders calls within one mock, so the existing reverse-order tests did not check order across layers.
…rder Cover the tagged increment path when the record expires while it is being read: the layer forgets the key instead of writing an expired record through tags. The lower-layer failure test asserted cleanup order with plain ordered(), which only orders calls within one mock. Use globally()->ordered() so it checks that the failed layer is cleaned up before the layer above it.
Plan for porting laravel/pennant as hypervel/pennant: coroutine-scoped memo and array storage, transaction-aware memo invalidation, tenant partitioning, batched and conflict-safe database reads and first resolution, an optional shared stored-value cache with token invalidation, StackStore batching, the Sentry integration, upstream fixes, tests and documentation, plus the owner-approved departures.
Start hypervel/pennant from laravel/pennant d9ea593 with the parts of the public surface that don't change shape: the Name attribute, the HasFeatures concern, the driver contracts, the events, GlobalScope, LazilyResolvedFeature and the helpers. Each file was copied from upstream and adapted for native types, method docblocks and imported class names. enum_value() stays an internal namespaced function without the function_exists() wrappers or the fallback branch, and casts integer-backed enum values to strings because feature names are strings. UsesDatabaseConnection is new. The decorator needs the connection a database driver uses for the current operation so values memoized inside a transaction can be forgotten when it rolls back. It is database-specific on purpose: an opaque identity wouldn't let the decorator find the transaction. FeaturePartitionNotResolved is new. A partitioned store whose resolver returns null or an empty string throws it instead of reading or writing across partitions. The "encoutered" typo in the null-scope event wording is fixed.
Laravel keeps memoized feature values on the store decorator and the array driver's values on the driver, and resets them between Octane requests and queued jobs. Hypervel stores are worker-lifetime services shared by concurrent requests, so that state moves to PennantState in CoroutineContext. Each request, job and child coroutine gets its own values, and no reset listeners are needed. PennantState holds the decorator memo, keyed by connection, store, partition, feature and scope; the array driver's values, keyed by store, partition, feature and scope; and the open transactions whose rollback must forget memoized values. forgetOnRollback() registers commit and rollback callbacks directly on the transaction record the first time a record is seen, and adds further connections to the same entry, since one record can cover several memo connections (an in-memory SQLite connection and its ::write alias share one owner). The callbacks capture the state and the record's object ID, not the record. replicate() copies the memo and array values into a copied context, except values for connections with an open transaction: the copy can't see that transaction roll back. It reads only its own tracked transactions, never the ambient context, because copyFrom() runs it in the destination coroutine. Transactions are never inherited. FeaturePartition is the resolved partition column and value for one operation. Its key() length-prefixes both parts, and casts the value to a string so 1 and '1', which select the same rows, share memo and cache identity.
The decorator memoizes values in PennantState instead of on the worker-shared instance. Its memo is keyed by connection, store, partition, feature and serialized scope, and presence uses array_key_exists() so stored null and false are hits. For drivers that implement UsesDatabaseConnection, every memo write made while the connection has an applicable transaction registers that connection on the transaction record, so a rollback (including a savepoint rollback) forgets values the request may have read or written inside it. Mutations remove the affected memo entries before calling the driver and write the new values only after it returns. flushCache() clears the store's memo and array values in the current coroutine only. The hot path works on plain arrays instead of Collections. Features and scopes are normalized once per operation, list-form entries share one default-scope resolution, each scope is serialized once, and a fully memoized getAllMissing() returns without calling the driver. Every event dispatch is guarded by Event::hasListeners(). Feature names for class strings are cached in classNames when the class is defined and cleared by setContainer(), since names come from container-built instances. Only Closures and feature classes are resolvers. Every other defined value is a constant, even when it looks like a callable or a class name. Upstream treated 'next', [$object, 'method'] and class-name values as resolvers in definedFeaturesForScope(), hasBeforeHook() and instance(), so they could drop out of all() and loadAll(), run a before() hook or return the wrong instance. define() now records class-backed features in featureClasses, and those methods read it. instance() also returns a closure that produces the defined value for null and class-name constants instead of the feature name. typeAllowsScope() matches builtin names with PHP predicates (mixed, object, iterable, callable, array, string, int, float accepting int, bool, true, false), resolves self and parent against the declaring class and uses instanceof for classes. Upstream rejected valid scopes for several of these. The "encoutered" typo in its exception message is fixed. Reflection is cached per feature: the resolver function in resolverFunctions and whether a before() hook accepts a null scope in beforeHookAcceptsNull. Both are cleared when the feature is defined again, and setContainer() clears the hook entries. A non-null before() result skips the driver entirely, so without the cache the reflection would repeat on every request. The array driver keeps its values in PennantState per store and partition, reads its state and partition once per operation through retrieve(), and implements CanSetManyFeaturesForScopes so setAll() no longer falls back to one set() per entry. It no longer captures a dispatcher: events go through the Event facade so fakes installed after the store resolves still apply. get() uses array_key_exists(), so a stored null is no longer resolved again. loadAll() used the first scope's features for every scope, so mixed User and Team scopes passed a Team to user-only resolvers. It now computes the valid features per scope and submits one sparse getAll(), keeping each scope's original position. Constructor departures for subclasses: the decorator drops $cache and takes a partition resolver; the array driver takes (string $name, ?Closure $partitionResolver, array $featureStateResolvers) and drops $events and $resolvedFeatureStates.
Reads. getAll() dedupes canonical (feature, serialized scope) pairs and groups features that share the same scope list, so a rectangular load() is one statement with F + S bindings. Groups are split and packed under QUERY_GROUP_LIMIT (500 OR groups, half SQLite's default expression depth) and QUERY_BINDING_LIMIT (10,000). The OR chain always sits inside a nested where so it can't escape the partition predicate, and rows are indexed by exact name and scope. Upstream's Collection::where() matched '01' to '1', and duplicate scopes produced duplicate inserts. First resolution. Missing pairs are resolved and inserted in WRITE_ROW_LIMIT chunks with insertOrIgnore() on PostgreSQL and SQLite, and a table-qualified no-op upsert on MySQL and MariaDB (INSERT IGNORE would truncate over-long values in strict mode). When the affected row count proves every candidate won, the candidates are returned, so the uncontended path keeps upstream's one select and one insert. Otherwise, or when MySQL may count duplicates as affected (CLIENT_FOUND_ROWS set in the connection options or a custom connector bound), the batch is reread from the write PDO and the decoded stored values are returned. MySQL and MariaDB add a shared lock because a REPEATABLE READ snapshot can't see the winner otherwise. Pairs that vanish before the reread are retried at most twice per operation, then 'Unable to insert feature values into the database.' is thrown. This replaces upstream's exception-driven retry, which shared a retry depth across calls and aborted PostgreSQL transactions on the unique violation. get() goes through getAll(), so the single-pair path shares the same insert and retry budget. Mutations. setAll() collapses duplicate pairs (last value wins, since PostgreSQL rejects an upsert that updates one row twice) and runs multi-chunk writes inside one transaction. setForAllScopes(), delete() and purge() are single statements with the partition predicate. Partitions. Every operation resolves the store's partition once and passes it down. newQuery() keeps upstream's base table query and newPartitionQuery() adds the partition predicate. Inserts and upserts key on [partition,] name, scope. connectionName() returns the requested connection, or the runtime default when the store's connection is null, so a null-connection store follows DB::usingConnection(). It rejects ::read connections before any memo, cache or SQL access: first resolution writes, and every statement on a read alias goes to a replica. StoredValueCache is an opt-in shared cache for stored row values, never before() results or unknown-feature false. Entries are keyed by the connection without its write suffix, the table and the partition, and are valid only while their partition and scope tokens match. Tokens are ULIDs written with the profile's TTL (DEFAULT_TTL 3600 seconds). Reads skip the cache inside a transaction and read from the write PDO when they publish, so a replica-lagged value is never published under a post-rotation token. Mutations rotate scope or partition tokens after commit in every invalidation target, attempt every target, and rethrow the first ordinary failure after the loop. A fill that captured tokens before a writer's commit publishes under rotated-away tokens, so readers ignore it without locks. The migration keeps text for value: jsonb and MySQL JSON reorder object keys, which breaks === comparisons between first resolution and later reads, and jsonb rejects \u0000. On MySQL and MariaDB, name and scope are VARBINARY so case, accents and trailing spaces can't make one scope read another's value. PennantMigration is ported as is. Constructor departures for subclasses: the driver takes (DatabaseManager $db, ?string $connection, string $table, ?Closure $partitionResolver, ?StoredValueCache $cache, array $featureStateResolvers) and drops $config, $events, $name and $retryDepth; retrieve(), insert() and the unused update() are gone and insertMany() is the conflict-safe insert.
FeatureManager stays a worker-lifetime auto-singleton. Upstream's closure binding is dropped because the container already shares unbound concrete classes. Config is read through named methods, and getDefaultDriver() reads pennant.default as a string with no code-level fallback, since the shipped config defines it. Stores read their pennant.stores config once, when built. Upstream (laravel/pennant#81) rereads the database store's connection and table on every query so a store kept across Octane requests follows config edits. Config is process-global in Hypervel, so request-time edits aren't supported anyway, and reading once matches CacheManager and skips the per-query lookups. Per-request connections use a store with a null connection under DB::usingConnection(), and shared tables use partitioning. After reconfiguring stores at boot or in tests, call forgetDrivers() and define features again. forgetDriver() and forgetDrivers() flush each discarded store's memo and array values so a rebuilt store can't inherit them. The database factory normalizes its config once: an empty or null connection means the runtime default, the table defaults to features, and a cache profile needs a store and a positive TTL (default 3600 seconds), because the cache treats null as forever and non-positive values as deletes. Each database store's shared cache targets the cache profiles of every configured database store on the same table whose connection overlaps it, so a write through an uncached alias still invalidates cached readers. resolvePartitionUsing() registers a column and a resolver at boot. It throws once any store is resolved, because an earlier store holds no partition resolver and would read and write across partitions. The column must be a plain SQL identifier. resolve() builds a per-store partition resolver, or none for stores configured with 'partitioned' => false, and passes it to the decorator and the built-in drivers. A null or empty partition value throws FeaturePartitionNotResolved. createArrayDriver() takes the config and store name, which resolve() already passed upstream. serializeScope() throws for a model without a key. Upstream stored every unsaved model under the same Class| scope, so they shared one stored value and one lottery draw. Worker-lifetime mutators carry Boot-only (extend, useMorphMap, resolveScopeUsing, resolvePartitionUsing, setDefaultDriver, EnsureFeaturesAreActive::whenInactive), Boot or tests only (forgetDriver, forgetDrivers) or Tests only (setContainer) warnings. EnsureFeaturesAreActive gets flushState() for its static response callback and reads app.debug with config()->boolean(). The provider merges pennant.stores by name like cache.stores, publishes the config and migration, registers the commands, the PublishingStubs listener and the Blade directives. Laravel's Octane and JobProcessed flush listeners aren't ported because feature values are memoized per coroutine. The feature stub drops its unused Lottery import.
Add the split package metadata and wire the package into the monorepo root: the replace entry, the Hypervel\Pennant\ PSR-4 mapping, the helpers.php and functions.php autoload files, the PennantServiceProvider discovery entry and the Feature alias. The provider is discovered rather than added to DefaultProviders, since not every application uses feature flags. The package declares only the dependencies its code imports: symfony/finder for discover(), hypervel/queue for the events' SerializesModels and hypervel/http for the middleware. It has no view requirement because the Blade directive closure's BladeCompiler type only loads when blade.compiler resolves. The README lists the deliberate differences from Laravel: keyless model scopes throw; stores read pennant.stores when first resolved, with DB::usingConnection() and partitioning in place of runtime config edits; and the built-in drivers and decorator take different constructor arguments, which affects subclasses but not custom drivers implementing the Driver contract.
Pennant keeps two pieces of process-global state that outlive the test application: the decorator's macros (Macroable stores them statically) and EnsureFeaturesAreActive's whenInactive() callback. Add flushPennantState() to the optional-package section, between Passport and Permission, and call both through callIfExists() so the subscriber still works when the package isn't installed. Decorator metadata and coroutine values need no reset here: the decorator belongs to the test container, and PennantState lives in the coroutine context the subscriber already flushes.
Port upstream's workbench models, factory and NewApi feature into
tests/Pennant/Fixtures, and its TestCase as a Testbench base that loads
the package migration. Upstream's unused createManager() is dropped.
Every upstream ArrayDriverTest and DatabaseDriverTest case is kept.
Adaptations:
- Config changes move from setUp() to defineEnvironment(), and
database setup runs in setUpInCoroutine() with RefreshDatabase.
- Incidental keyless User and Team scopes get explicit ids with
unchanged assertions; testItRejectsModelsWithoutAKeyAsScope covers
the intentional difference.
- The unknown-feature event tests also fake the event after the store
has resolved, since the drivers dispatch through the Event facade.
- The updated-connection test calls forgetDriver('database') instead of
a second flushCache(), because stores read their config when built.
- The migration-connection test extends PennantMigration instead of
copying it into a plain Migration.
- @laravel.com emails become @hypervel.com.
- The four UniqueConstraintViolationException listener tests simulated
conflicts the new insert no longer raises. They now cover a
concurrent winner stored before the insert (its decoded value is
returned, including an array winner for an object candidate) and a
winner that vanishes before the reread (three attempts, then the
insert failure message with no previous exception), for both batch
and get().
- testItHandlesNullScopeForBeforeHook asserted the first read twice;
its second assertion now checks the second read.
Regression tests cover the upstream fixes in this port: constant values
that look like callables or class names stay constants in all(),
loadAll(), before() hooks and instance(); instance() of a null
constant returns a closure producing null; a null resolver result is
stored and not resolved again across getAll(); redefining a feature
class uses its current name; and redefining a feature rechecks whether
its before() hook accepts a null scope.
Port every upstream case from DefinesFeaturesExternallyTest, EnumTest,
FeatureDirectiveTest, FeatureHelperTest, FeatureManagerTest,
FeatureMiddlewareTest, HasFeatureTest, PurgeCommandTest and
IntersectionTypeTest, with these adaptations:
- The external-definition drivers extend ArrayDriver through its
Hypervel constructor ('external', null, []) instead of passing the
event dispatcher.
- PurgeCommandTest's custom store was an anonymous object that didn't
implement Driver, which the typed store creator rejects. It is now an
ArrayDriver subclass with a no-op purge().
- IntersectionTypeTest drops the PHP 8.2 RequiresPhp attribute, since
Hypervel needs PHP 8.4, and gives its User and Team scopes ids.
Record the design settled while porting the source and upstream tests: - Stores read pennant.stores when built, with forgetDrivers() and redefinition after boot or test reconfiguration, in place of upstream's per-query config reads. - The partition is resolved once per operation and passed down (newPartitionQuery(), ArrayDriver::retrieve()), and resolvePartitionUsing() throws once a store is resolved. - Only Closures and feature classes are resolvers; featureClasses replaces guessing from nameMap values (upstream fix 8). - Feature names and before-hook null acceptance are cached per feature and cleared on redefinition and container replacement. - The conflict reread returns decoded stored values. - The default TTL is documented in pennant.md rather than the config comment. - The test adaptations and the README differences. The status now points at the new tests.
cacheProfile() only rejected TTLs below one, so a float such as 1.5 or a numeric string such as '60' passed. The cache repository's put() and putMany() take int|null and StoredValueCache calls them under strict types, so these values raised a TypeError later. For a store's own cache, the TypeError came from the StoredValueCache constructor when the store was built. A profile that another store only uses as an invalidation target reached a typed parameter only during rotation, which runs after the write commits: every mutation through that other store committed and then threw. Every profile, including invalidation-only targets, is now checked to be an integer of at least one while the store is built, before anything can be written. The message now says "positive integer".
CoroutineIsolationTest covers the per-coroutine PennantState: - Concurrent coroutines don't see the parent's array values, resolve and activate independently, and leave the parent's value unchanged. - A copied context starts from the parent's values, and the child's writes don't reach the parent. - Forking with a copied context, or calling copyFrom(), inside an open transaction doesn't copy values memoized in that transaction, both for driver values and for before() results. - Switching partitions inside one coroutine keeps separate values. - A null-connection database store keeps separate values for each connection selected with DB::usingConnection(). TransactionTest covers the memo's transaction tracking and the shared cache's commit handling: - Rolling back forgets values memoized inside the transaction, inside a rolled-back savepoint, and inside savepoints committed under an outer transaction that rolls back. - before() results are forgotten on rollback too. - Values memoized through base and ::write stores that share one in-memory SQLite transaction are all forgotten. - Committing keeps memoized values without another query. - Flushing one store keeps the rollback tracking of other stores. - A multi-chunk setAll() rolls back together when a later chunk fails. - The shared cache is bypassed inside a transaction, invalidated only after the outermost commit, and left valid when the transaction rolls back.
The tests use a team_features table partitioned by team_id, a resolver that returns the current team, a cached store on the same table and an unpartitioned store on the features table. They check that: - Reads, grouped reads and first resolutions stay in the current partition, and only partitioned stores call the resolver. - set, set many, set for all scopes, delete, purge of named features, purge of everything and pennant:purge only change the current partition's rows. - stored() lists only the current partition's features. - A store with partitioned => false never calls the resolver. - A null or empty-string partition throws FeaturePartitionNotResolved. - Integer 1 and string '1' share memo and shared cache identity. - Different partitions don't share cached values or invalidation: after one team changes through the uncached store, the other team's value is still a cache hit. - Registering the resolver after a store has resolved throws. - Qualified names and expressions are rejected as partition columns.
The tests use worker-array caches so they run without Redis: - A value read before a write but published after it is ignored, since the write replaced the tokens it was read under. - Writes through an uncached store invalidate only their own scope. A data provider covers the default connection and base/::write reader-writer pairs with explicit connection names. The reader uses its own cache store, so target matching and cache key identity must both strip ::write; the null-connection store's profile would otherwise be a target of every writer and hide a broken match. - Writes to every scope invalidate the whole partition. - Unknown feature values are never cached. - Objects and JsonSerializable values read back from the cache as the same arrays the database returns, and false and null are still cache hits that run no query. - Memo hits touch neither the database nor the cache, and cache hits run no query and hold no pooled connection. - When new tokens can't be stored, nothing read in that operation is published. - After a committed write whose invalidation fails, the writer's next read returns the updated row rather than a stale memo value. - A target that returns false or throws doesn't stop the remaining targets, and its failure is rethrown. - Zero, negative, float and numeric-string TTLs are rejected.
DatabaseDriverTest: - '1' and '01' are matched as different scopes, not loosely compared. - Features sharing a scope list are read together: a rectangular batch uses one query with F+S bindings and a sparse batch one query with the bindings of each group. - Reads split into a second query past QUERY_GROUP_LIMIT groups and past QUERY_BINDING_LIMIT bindings. - A repeated scope is read and inserted once. - A ::read connection is rejected before any query, whether configured on the store or selected with DB::usingConnection(). - loadAll() with User and Team scopes gives each scope only the features whose resolvers accept it, keeping the scope positions. - setAll() with a repeated feature and scope keeps the last value. ArrayDriverTest: after forgetDriver() or forgetDrivers(), a rebuilt store resolves the redefined value instead of the forgotten store's memoized one. FeatureManagerTest: one data provider checks isResolverValidForScope() for object, iterable, callable, float with an int, standalone true and false, and self/parent parameter types, with negative cases.
The database tests run Pennant's own migration on each engine. Shared by MySQL, MariaDB and PostgreSQL: - Identifiers that differ only by case, accents, trailing spaces or composed/decomposed form are stored and read separately, as names and as scopes. This covers the VARBINARY columns on MySQL and MariaDB, whose default collations treat these as equal. - 255 four-byte characters fit in both name and scope. - An uncontended first resolution uses one read and one insert. - A row stored through a competing connection between the read and the insert is returned as the winner. MySQL and MariaDB also return the winner when FOUND_ROWS is enabled through the connection options or through a bound connector, and inside a REPEATABLE READ transaction, where only the locking reread can see it. MySQL also covers use_upsert_alias. PostgreSQL covers activating a repeated scope in one batch, which it would reject as an upsert touching one row twice, and checks that a serialization failure inside a REPEATABLE READ transaction propagates. The Redis test covers publishing, scope invalidation and bulk invalidation (setAll, activateForEveryone and purge) through the public API with a fresh memo. Its directory is added to the Redis, Redis Cluster and Valkey workflow jobs; the database directories are found by convention.
Step 5 is complete. The test list now describes the partition cache isolation case, the base/::write reader-writer pairs and their separate reader cache, and how cached objects, JsonSerializable values, false, null and floats read back. The next step is the Sentry integration.
Ports PennantPackageIntegration from sentry-laravel, including the upstream fix that replaces backslashes with dots so class-based feature names are accepted by Relay. Each FeatureRetrieved event records the flag on the current Sentry scope, as active unless its value is false, so error events show which flags were checked and their results. Hypervel features take no arguments in onBoot(), so the integration resolves the event dispatcher from its container like the other features. The listener parameter is typed, and the unused FEATURE_KEY constant and FeatureResolved import are not ported. Sentry's scope is stored per coroutine, so flags never cross requests, and the SDK keeps at most 100 flags per scope. The integration is added to the shipped features list after NotificationsIntegration, following upstream's order. It applies only when Pennant is installed, so the Sentry package gains no dependency. All five upstream tests are ported; the class_exists skip is dropped because Pennant is always installed in this repository. The Sentry docs introduction mentions the flags, and the Pennant item is removed from docs/todo.md.
FeatureScopeable::toFeatureIdentifier() receives the name of the store being used: FeatureManager passes the store name to the Decorator, and resolveScope() passes that name on. The docblock said "driver", so a store named "tenant" using the database driver would not match code written against driver names. Laravel behaves the same way. The docblock now describes the store name. The parameter keeps its upstream name, $driver, so named arguments stay compatible.
pennant.md starts from Laravel's Pennant docs, with namespaces, package names and source links pointing at Hypervel, date examples using the Date facade and missing example imports added. The before() examples now return null explicitly, since a mixed return type can't fall through, and read config through typed getters. Hypervel-specific additions: - Installation: binary name and scope columns on MySQL and MariaDB. - Configuration: null connections follow DB::usingConnection(), ::read connections are rejected, stores read their config once, and reconfiguring needs forgetDrivers() and new definitions. - Feature classes are shared by the worker, so they hold no request state, and before() can defer to an entitlement or billing service. - In-memory cache: per coroutine, copied only with the context, and for database stores forgotten on rollback and not copied while a transaction is open. - Scopes: the supported scope types, keyless models throwing, the store name passed to toFeatureIdentifier(), and the stored false of unexpected null scopes. - all() can include classes registered by earlier requests on the worker; discover() gives a stable set. - New Partitioning and Shared Cache sections, plus custom driver guidance, worker-wide DynamicallyRegisteringFeatureClass, and Credits. The README's differences now link to the configuration docs and name the changed protected methods and FeatureManager. The porting guide gains a Pennant entry, and the documentation index lists Pennant.
Steps 6 and 7 are complete. The next step is the step 8 verification and the full package review.
Step 8 named the shared database test script, which reruns the whole database integration suite and every package's engine directories. The change only adds Pennant's engine and Redis directories, so the step now names those. Verification is complete; the next step is the full package review.
Every step is implemented, verified and committed, and the full package review is signed off. The next step is owner review.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoPort Laravel Pennant with coroutine-safe feature flags
AI Description
Diagram
High-Level Assessment
Files changed (86)
|
Code Review by Qodo
1. An opt-out store can delete tenant flags
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
 [High impact] The reviewed changes appear safe to merge; no new actionable issues remain. SummaryPorts Laravel Pennant with per-coroutine feature values, transaction-aware caching, partitioning, and a shared cache.
Reviews (3) · Last reviewed commit: "Return Pennant feature names as strings" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The upsert alias test enabled use_upsert_alias on every MySQL server, but the INSERT ... AS alias syntax only exists from MySQL 8.0.19, so MySQL 5.7 rejected it with a syntax error. The test now requires 8.0.19 or later. The REPEATABLE READ winner test was shared by MySQL and MariaDB. MariaDB 11.6.2 and later turn innodb_snapshot_isolation on by default, and with it a REPEATABLE READ transaction that runs into a row committed after its snapshot fails with error 1020. That is the same contract as PostgreSQL's serialization failure: the caller's transaction has to restart, and the framework already treats the error as retryable. The driver is right to let it through, so only the test was wrong. The shared test now lives in the MySQL test class. MariaDB gets two tests that set the variable explicitly: with it off, the locking reread returns the concurrent winner; with it on, the 1020 error reaches the caller. Releases that predate the variable behave as if it were off, so the first test runs there without setting it and the second is skipped.
for() merged the given scopes with array_merge(), which keeps string keys. A keyed collection such as collect(['admin' => $user]) therefore left no scope at index 0, and value() and all() failed with an undefined key. A second for() call with the same key also replaced the earlier scope instead of adding another one. The incoming scopes are now reindexed before they're merged. The array driver test also fixes a copy-paste mistake inherited from upstream: the activate-and-deactivate test checked bar twice and never checked baz.
The command deduplicated --except with a loose unique(), so --except=01 --except=1 kept only one of the two names and the other feature was purged. It then flipped the feature list into array keys, which turned a numeric-string name such as "2" into an integer that was passed on to the driver and the FeaturesPurged event. The exclusions are now applied with array_diff(), which compares names as exact strings and returns them unchanged.
purge([]) ran a delete that matched nothing, then rotated the partition's shared-cache token, which invalidated every cached value in the partition on every server. pennant:purge reaches it when --except leaves nothing to purge. setAll([]), which Feature::activate([]) reaches, borrowed a connection to schedule an invalidation with no scopes. Both now return early. The decorator still dispatches its events as upstream does.
putMany() computed one absolute expiration before writing the first layer and gave every layer the full TTL. A lower layer written later could therefore keep a record past the expiration stored inside it; increment() would then delete the key instead of updating it, and backfill would skip it. Each layer's records are now built when that layer is written, from the current time and the full TTL. That is what put() already does for a single key, so each layer's stored expiration matches its own TTL and every layer still receives the new value. Compensation after a failed layer still forgets the original batch keys.
PennantMigration::getConnection() returned the default connection's name when the database store had no connection. The migrator treats any non-null name as the migration's own connection, so migrate --database=X created or dropped the features table on the default database instead of X. It also treated the string 'null' as unset, which the store itself doesn't, so the two could disagree. It now returns null for a null or empty connection, matching how the store normalizes it, and keeps explicit names. Without --database the migration still runs on the default connection. Upstream's test now covers null and empty values through the real migrator, plus an explicit connection. The driver test also fixes the same upstream copy-paste mistake as the array driver test: the second bar assertion in each block now checks baz.
Until the application has booted, Hub::configureScope() edits the baseline scope that every later request and job copies. A feature checked in a provider's boot() method was therefore recorded on that baseline and appeared on every request's Sentry events. The integration now registers its listener once the application has booted, so boot-time checks are never recorded and feature checks pay no extra cost.
Shared-cache keys contained the raw scope and feature name. A long scope or feature, which the features table allows, pushed the key past the database cache store's 255-character key column, and every feature check through that store failed with a database error. Keys are now the entry kind plus a SHA-256 hash of the same unambiguous, length-prefixed identity: connection, table, partition, and the scope and feature where they apply. SHA-256 rather than a fast hash keeps one scope from being crafted to share another scope's cached value, and an unseeded hash is shared by every server. The new integration test caches a 255-character feature and scope through the database cache store on each engine.
With task_enable_coroutine off, which is the default, task listeners run outside a coroutine and share the worker's single non-coroutine context. Anything a task kept in coroutine context, such as Pennant's feature values, authentication state or scoped instances, carried over into every later task in that worker. Coroutine tasks don't have this problem because each starts in a fresh coroutine. TaskCallback now saves the worker's context, runs the task and its TaskTerminated listeners in an empty context, then puts the worker's context back before rethrowing any failure. TaskTerminated listeners such as the database and Redis connection releases still see the task's own state. Core declares its new context and coroutine dependencies.
The Driver contract now says that purge(null) purges every feature, which both built-in drivers already do and custom drivers need to know. The rich values docs explain that the database driver stores values as JSON, so reading one back can change its PHP type. The shared cache docs no longer promise that a cache hit runs no query: with the database cache store it still queries the cache table. They now say that a hit avoids reading Pennant's feature table, and that with Redis or Swoole it also avoids taking a database connection. The middleware test for one inactive feature now activates the other feature, so it can tell "all active" apart from "any active".
pennant:purge compared the raw command input against the names Pennant stores, but Feature::purge() resolves each feature to its registered name before deleting it. A feature class with a custom name was therefore purged even when it was explicitly excluded: - `--except='App\Features\NewApi'` didn't match the stored name `new-api`, so `new-api` was purged. - `'App\Features\NewApi' --except-registered` kept the class string, which purge() then resolved to `new-api` and deleted. Both sides of the comparison now go through the store's name() method, the same resolution purge() uses. Stored names are resolved too, so rows written under a class string before the class gained a custom name can't delete an excluded registered name. The list of registered features for --except-registered is still taken before any input is resolved. Names are still compared exactly, so "01" and "1" stay distinct. When exclusions apply, the success message now lists the registered names that were purged.
The driver contracts promise string feature names, but the built-in drivers built defined() (and the array driver's stored()) from array keys. PHP turns numeric keys such as "1" into integers, so a feature named "1", including one named by an integer-backed enum, came back as the integer 1. The Decorator compared names against defined() with a loose in_array(), which hid this but treated "01" and "1" as the same name. A feature class named "1" was never registered when a feature named "01" already existed, so checking it returned the unknown-feature result instead of its own value. Switching to a strict comparison alone would have made a feature class named "1" replace an existing "1" definition. The drivers now cast the names they return to strings, and both Decorator checks compare strictly. A feature class with an integer-backed name is registered beside "01" and keeps using an existing "1" definition.
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
1 participant
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.
This adds
hypervel/pennant, a port of Laravel Pennant (ported fromd9ea593). The public API is the same as Laravel's: feature definitions, class-based features, scopes, rich values, eager loading, theHasFeaturestrait, the Blade directive, the middleware, events and the Artisan commands all work the way Laravel developers expect.The internals needed more than a namespace change. Pennant assumes PHP's request lifecycle: it caches resolved values on a long-lived object and relies on Octane listeners to clear them between requests. In a Hypervel worker many requests run at the same time, so that cache would leak one user's flags into another user's request. Fixing that properly touched most of the package, and while I was in there I fixed several upstream bugs and added two features that matter for applications running on more than one server or for more than one tenant.
Coroutine-safe state
Pennant's in-memory cache and the
arraydriver's values now live in coroutine context instead of on the store objects. Each request, queued job and command gets its own values, and nothing needs to be cleared between them. That's why Laravel's Octane andJobProcessedflush listeners aren't ported. Child coroutines that copy their parent's context get their own copy of the values.The stores themselves, feature definitions and resolver metadata stay worker-lifetime, since they're the same for every request. Feature classes are resolved once per worker, like policies and listeners, so the docs ask for stateless feature classes.
Transactions
Upstream keeps a value in its in-memory cache even when the transaction that read or wrote it rolls back, so the rest of the request sees a value that was never stored. Values read or written inside a database transaction are now dropped from the in-memory cache when that transaction (or savepoint) rolls back, and they aren't copied into child coroutines while the transaction is still open.
Faster feature checks
A cached
Feature::active('x')call in upstream creates many short-lived collections, scans the cache several times and always dispatchesFeatureRetrieved. Feature checks run on hot paths, so the decorator and scoped interactions now work on plain arrays, normalize each feature and scope once per call and read the in-memory cache directly. Feature names, class-based feature metadata andbefore()hook signatures are looked up once per worker. Every event dispatch is skipped when nothing is listening.Database driver
Batched reads. Loading several features for several scopes no longer means one query per pair. Pairs are grouped by their scope list, so loading F features for S scopes is one query with F + S bindings, and large batches split under SQLite's expression depth and binding limits.
Safe concurrent first resolution. When two requests resolve the same feature at once, upstream inserts, catches the unique constraint violation and retries. On PostgreSQL that violation aborts the surrounding transaction, so the retry fails too. First resolution now uses
ON CONFLICT DO NOTHINGon PostgreSQL,INSERT OR IGNOREon SQLite and a no-op upsert on MySQL and MariaDB. When some rows weren't inserted, the driver reads back the stored values so every request agrees on the winner. MySQL and MariaDB use a locking read for this, because a REPEATABLE READ snapshot can't see the competing row otherwise, and the driver also rereads whenFOUND_ROWSmay make the affected row count unreliable. A value that disappears between the insert and the reread, such as through a concurrent purge, is resolved again a bounded number of times before the driver gives up with an exception. The driver never returns a value it doesn't know was stored.Exact identifiers on MySQL and MariaDB. The default
utf8mb4_unicode_cicollation treatsTeam-A,team-a,Team-Aandcafé/cafeas equal, so upstream can return another scope's stored value. The migration usesVARBINARYfornameandscopeon those databases.utf8mb4_binisn't enough, since it still ignores trailing spaces. PostgreSQL and SQLite already compare exactly.valuestays a text column. It isn'tjsonb: PostgreSQL'sjsonband MySQL'sJSONreorder object keys, which breaks===comparisons between the first resolution and later reads of a rich value, andjsonbrejects\u0000. Pennant never queries inside values, so the column type guideline inAGENTS.mdnow allowstext()for opaque serialized values.Other fixes. Rows are matched exactly instead of with Collection's loose comparison, which matched scope
'01'to'1'. Duplicate pairs in one batch no longer cause duplicate inserts, andsetAll()keeps the last value for a duplicate pair instead of failing on PostgreSQL. LargesetAll()calls run in one transaction.Read connections are rejected. First resolution writes, and every statement on a
::readconnection goes to a replica, so a store configured with a read connection (or used insideDB::usingConnection('mysql::read', ...)) throws before touching the database.Config is read once. Upstream rereads the database store's connection and table on every query so a store kept across Octane requests follows config changes. Config is shared by every request in a Hypervel worker, so it should only change during boot or in tests. Stores read their config when they're built, and the docs explain how to use
DB::usingConnection()for per-request connections and partitioning for shared tables.Partitioning
Applications with tenants, workspaces or teams often want each one's feature values kept apart in a single table. Putting the tenant into every scope gets part of the way there, but global flags, purges and cached values would still cross tenants. Partitioning makes it part of the store:
Once a partition is registered, every operation applies it: reads, first resolutions, updates, bulk updates, purges (including
pennant:purge), the in-memory cache, thearraydriver and the shared cache. Anullor empty partition value throwsFeaturePartitionNotResolvedinstead of reading or writing outside a partition, and registering a partition after a store has been resolved throws, since that store would already be working across partitions. A store with'partitioned' => falseopts out, for flags shared by every partition. The partitioned schema is up to the application; the docs show the migration change.Shared cache
Each new request starts with an empty in-memory cache, so its first check of every feature is a database query. A database store can now share stored values between servers through a cache store:
A cache hit runs no query and doesn't take a connection from the pool. The database stays the source of truth: only stored values are cached, never
before()results or thefalseof unknown features, and an evicted value is read from the database again rather than resolved again, so the cache can never change a stored assignment.Invalidation uses random tokens per partition and per scope. A cached value is only valid while the tokens it was saved under are current, and every update replaces the affected tokens after its transaction commits. A request that read the database before an update and saves the old value afterwards saves it under replaced tokens, so nobody reads it. That avoids locks and compare-and-swap. Updates through any database store on the same table and connection invalidate every cached store that reads it, even when the store making the change has no cache of its own. The shared cache isn't used inside a transaction, so values that might roll back are never published.
If invalidation fails after a commit, Pennant throws, and other servers may read the old value until the TTL expires. The TTL must be a positive integer for that reason, and the docs recommend a Redis store when changes must reach every server promptly.
Other upstream fixes
Each of these has a regression test.
arraydriver treated a storednullas missing and resolved it again.object,iterable,callable,self/parent, standalonetrue/false, andintscopes forfloatparameters.loadAll()used the first scope's features for every scope, so mixedUser/Teamscopes passed aTeamto user-only resolvers.'next'orSomeClass::class, were treated as resolvers inall(),loadAll(),before()hook detection andinstance().Differences for subclasses
The built-in drivers, the store decorator and
FeatureManager::createArrayDriver()take different constructor arguments, and a few protected methods changed, because values live in coroutine state and stores can be partitioned. Custom drivers that implement theDrivercontract are unaffected. The package README lists these differences, and the porting guide covers keyless model scopes, the migration and runtime config changes.Changes outside Pennant
many()andputMany()per layer. Both used to loop over keys, so a cold Pennant batch on a Swoole table + Redis stack cost one Redis round trip per key. Each layer is now called once, with lower-layer hits written back to the layers above, keeping their expiration. When a layer'sputMany()fails, the batch keys are removed from that layer and the ones already written, so the stack doesn't serve values that only reached some layers.PennantPackageIntegrationis ported fromsentry-laraveland enabled in the shipped config, so Sentry events include the flags checked during the request.AfterEachTestSubscriberflushes Pennant's static state when the package is installed.Not ported
JobProcessedflush listeners, since coroutine state replaces them.Documentation
pennant.mdis adapted from Laravel's docs, with new sections for partitioning and the shared cache, and notes on the per-coroutine in-memory cache, transactions, stateless feature classes, keyless model scopes and store configuration. Pennant is added to the docs index and the porting guide, and the Sentry docs mention feature flags.Tests
The upstream suite is ported in full. Upstream tests that used unsaved models as incidental scopes now give them ids, with assertions unchanged, and a separate test covers the keyless model exception. The tests that simulated unique constraint violations now cover what can actually happen with the new insert: a concurrent winner's value is returned, and a winner that vanishes before the reread ends in an exception after the bounded retries.
New tests cover coroutine isolation, transaction rollback, partitioning and the shared cache. Integration tests run on MySQL, MariaDB and PostgreSQL for exact identifiers and each engine's conflict paths, and on Redis for the shared cache. The Redis directory is added to all three jobs in the Redis workflow.
Validation
Summary by cubic
Ports Laravel Pennant as
hypervel/pennantwith the same public API, adapted to Hypervel's shared worker processes. Feature values are now kept per coroutine instead of being flushed between requests, and values read inside a transaction are forgotten on rollback.migrate --database.Driverimplementations are unaffected.StackStorebatchesmany()andputMany()per layer with per-layer expiration; Sentry events include checked feature flags but skip boot-time checks."1"stay distinct from"01", andpennant:purge --exceptmatches exclusions by registered name.Written for commit 4982ecc. Summary will update on new commits.
Note
Port Laravel Pennant to Hypervel as the
hypervel/pennantpackageFeatureManager,ArrayDriver,DatabaseDriver,Decorator,PendingScopedFeatureInteraction, Blade conditionals,EnsureFeaturesAreActivemiddleware,pennant:featureandpennant:purgecommands, afeature()helper, and thefeaturestable migration.PennantState(PennantState.php); memoized values are cleared on transaction rollback and are not copied into child coroutines when created inside an open transaction.StackStore::manyandStackStore::putManyin StackStore.php to use batched reads/writes per cache layer, and fixes non-coroutine task callbacks in TaskCallback.php to run with a cleared context and restore the worker context afterward.many/putManyinstead of per-key calls.Macroscope summarized 4982ecc.