Skip to content

Add scope column for categorising feature flags#9

Open
stevethomas wants to merge 3 commits into
mainfrom
v2/scope-column
Open

Add scope column for categorising feature flags#9
stevethomas wants to merge 3 commits into
mainfrom
v2/scope-column

Conversation

@stevethomas
Copy link
Copy Markdown
Member

Summary

Re-opened from #6. Originally merged but reverted from main to keep v1.x clean — this is targeted at v2.

  • Adds scope and description columns to the features table
  • Supports rich config format alongside simple FeatureState values
  • Backed enum support for scope values
  • Sync action updates scope/description on re-sync without touching state

Test plan

  • Existing tests pass
  • New scope/description tests pass
  • Migration runs cleanly on fresh database

JonathanLouw and others added 3 commits April 16, 2026 16:33
* add scope column for categorising feature flags

Adds a nullable scope string column to the features table, allowing
projects to categorise flags however they want (e.g. 'development',
'release'). Supports both simple and rich config formats, with
BackedEnum resolution. Scope syncs from config on every deploy.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix import ordering in SyncFeaturesActionTest

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* document scope feature and upgrade steps in README

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* rewrite scope README section to focus on practical use case

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* add description column, publish-only migrations, and v2 docs

- Add nullable `description` column to features table for human-readable
  flag explanations in admin UIs
- Switch to publish-only migrations (no auto-loading from vendor)
- Delete redundant alter migration; single create stub is source of truth
- Sync scope and description from config on every deploy (state still
  only set on creation)
- Add `getDescription()` to FeatureFlags and facade
- Add UPGRADING.md for v1 to v2 migration guide
- Rewrite Feature Scopes README section to frame scope as categorisation
  for UIs, not runtime behaviour
- Update config docblock for rich format with description

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
These accessors have no practical use case — scope and description
are for bulk filtering via Eloquent, not single-feature lookups.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

return [
'name' => $name,
'state' => $alwaysOn ? FeatureState::on() : $config['state'],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The normalise method accesses keys on the $config variable without first validating that it is an array and contains required keys, risking a crash during synchronization.
Severity: HIGH

Suggested Fix

Before accessing array keys on the $config variable within the normalise method, add a validation step. Check that $config is an array and that it contains the required 'state' key. If the validation fails, throw a descriptive InvalidArgumentException to inform the user of the specific configuration error.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/Actions/SyncFeaturesAction.php#L66

Potential issue: In the `SyncFeaturesAction::normalise` method, the `$config` variable,
which comes directly from user configuration, is accessed as an array without prior
validation. If a user provides a rich-format configuration as an array but omits the
required `'state'` key, the access on line 66 (`$config['state']`) evaluates to `null`.
This `null` value is then passed downstream, causing an `InvalidArgumentException` in
the `FeatureStateCast::set` method, which crashes the entire feature flag
synchronization action. A similar fatal error occurs if the config value is not an array
at all (e.g., a string or `null`), as the code will attempt an illegal offset access.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants