Skip to content

Conversation

@ljonesfl
Copy link
Member

@ljonesfl ljonesfl commented Jan 15, 2026

Note

Fix:

  • Updates Base::initSettings to detect and use an existing SettingManager directly; otherwise wraps a raw source in SettingManager. Ensures env fallback via Env and maintains base_path handling.

Tests:

  • Adds tests validating direct use of a provided SettingManager, wrapping of raw Ini sources, and automatic env fallback setup in tests/Application/ApplicationTest.php.

Docs/Chore:

  • Updates versionlog.md with 0.8.16 entry describing the fix.

Written by Cursor Bugbot for commit 67671b8. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • Bug Fixes

    • Settings initialization now accepts existing configuration managers directly, preventing double-wrapping.
  • Tests

    • Added unit tests covering settings initialization: direct manager usage, wrapping raw sources, and environment fallback behavior.
  • Chores

    • Cleaned up the version log and added a 0.8.16 entry.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

initSettings in src/Application/Base.php now detects if the provided $source is already a SettingManager and uses it directly; otherwise it wraps the raw source in a new SettingManager. Tests and a changelog entry for 0.8.16 were added/updated.

Changes

Cohort / File(s) Summary
SettingManager Wrapping Logic
src/Application/Base.php
initSettings now checks instanceof SettingManager and uses it directly when true; otherwise constructs a new SettingManager around the raw source. Existing base path and Env fallback logic remain.
Unit tests
tests/Application/ApplicationTest.php
Added three tests: testInitSettingsWithSettingManagerUsesItDirectly, testInitSettingsWithRawSourceWrapsIt, and testInitSettingsSetsEnvFallbackWhenNoneProvided to verify wrapping behavior and fallback provisioning.
Changelog Maintenance
versionlog.md
Added 0.8.16 entry ("Fix to support setting managers or sources correctly...") and removed several empty/placeholder version sections.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Src as Source (e.g., Ini)
    participant SM as SettingManager
    participant Env as Env fallback

    rect rgba(200,200,255,0.5)
    App->>Src: initSettings($source)
    alt $source is SettingManager
        Src->>App: return SettingManager (used directly)
        App->>SM: ensure Env fallback set if missing
        SM->>Env: attach fallback
    else $source is raw source
        App->>SM: new SettingManager($source)
        SM->>Src: wrap raw source
        SM->>Env: attach fallback if missing
    end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰
I checked the wrapper, soft and neat,
If it's already snug, I skip the repeat.
Fallbacks stay cozy, no nests undone,
Tests hop in line—one, two, three—then run! 🥕🐇

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main change: modifying initSettings to support accepting a SettingManager object instead of always wrapping the source.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1a770a and 67671b8.

📒 Files selected for processing (2)
  • tests/Application/ApplicationTest.php
  • versionlog.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/Application/ApplicationTest.php
  • versionlog.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cursor Bugbot

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@versionlog.md`:
- Around line 1-2: The changelog entry is truncated; update the "## 0.8.16"
bullet to fully describe the fix by replacing "Fix to support " with a clear
sentence such as "Fix to support passing a SettingManager instance directly to
initSettings (accept SettingManager objects in initSettings)"; ensure the
wording references initSettings and SettingManager so readers know what changed
and keep the markdown list formatting consistent.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5797cc2 and 9714c00.

📒 Files selected for processing (2)
  • src/Application/Base.php
  • versionlog.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
src/Application/Base.php (1)

720-729: Verify that SettingManager class exists and implements ISettingSource.

The logic correctly avoids double-wrapping when a SettingManager is already provided, while maintaining backward compatibility for raw ISettingSource implementations. However, the SettingManager class definition and its interface compliance must be confirmed as a prerequisite for this code to function. Ensure the dependency is properly declared and available.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@codecov
Copy link

codecov bot commented Jan 15, 2026

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@tests/Application/ApplicationTest.php`:
- Around line 577-593: The test method
testInitSettingsWithSettingManagerPreservesFallback is misleading because it
verifies that initSettings sets an Env fallback when none exists, not that it
preserves an existing fallback; rename the test to
testInitSettingsSetsEnvFallbackWhenNoneProvided and update the inline comment
("Create a SettingManager with custom fallback") to reflect that we're creating
a SettingManager without a fallback and verifying AppMock::initSettings (via new
AppMock() and getSettingManager()) sets an Env fallback; leave the assertions
using getFallback() and InstanceOf(\Neuron\Data\Settings\Source\Env::class)
unchanged.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9714c00 and e1a770a.

📒 Files selected for processing (1)
  • tests/Application/ApplicationTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
tests/Application/ApplicationTest.php (2)

538-558: Well-structured test for direct SettingManager usage.

The test correctly validates that when a SettingManager is passed directly, it's used as-is without double-wrapping. The combination of assertSame for identity and checking the internal source type is thorough.


560-575: Good complementary test for raw source wrapping.

This test validates the other code path where a raw ISettingSource is wrapped in a SettingManager. Together with the previous test, these provide good branch coverage.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@ljonesfl ljonesfl closed this Jan 15, 2026
@ljonesfl ljonesfl deleted the feature/cli-boot-fix branch January 15, 2026 19:43
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