Skip to content

Document code review findings and potential null-handling bug in Config#19

Draft
Copilot wants to merge 3 commits intomasterfrom
copilot/code-review-project
Draft

Document code review findings and potential null-handling bug in Config#19
Copilot wants to merge 3 commits intomasterfrom
copilot/code-review-project

Conversation

Copy link

Copilot AI commented Jan 29, 2026

The task was to review the Config package, summarize pros/cons, and surface any likely bugs. Added unit coverage to illustrate specific edge behaviors.

  • Strengths

    • Clear nested access API (dot-notation, array accessors), rich traversal hooks (SignalCode), solid test coverage, and strong Psalm typing with generics.
  • Concerns

    • has() treats stored null as “missing,” so callers cannot distinguish “present null” vs “absent.”
    • Scalar-to-array coercion during lookup can yield surprising paths; deletes can silently succeed when paths are malformed.
    • No delimiter escaping; keys containing dots are only reachable via array path form.
  • Potential Bug (null handling)

    • Keys explicitly set to null are reported as absent:
      $cfg = new \ItalyStrap\Config\Config(['flag' => null]);
      $cfg->has('flag');            // false (unexpected if null is a valid value)
      $cfg->get('flag', 'default'); // 'default' instead of null
  • Additional Coverage

    • Added tests showing scalar-to-array coercion during nested access leaves the scalar unchanged but returns defaults for deeper paths.
    • Added tests showing keys containing the delimiter must be accessed via array-path form; dot-notation interprets them as nested paths.
Original prompt

Do a code review of this project and provide me a list of pros and cons. If you spot any bug let me know.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: overclokk <4604932+overclokk@users.noreply.github.com>
Copilot AI changed the title [WIP] Review project code for pros, cons, and bugs Document code review findings and potential null-handling bug in Config Jan 29, 2026
Copilot AI requested a review from overclokk January 29, 2026 10:23
@overclokk
Copy link
Contributor

@copilot
has() treats stored null as “missing,” it is correct, in fact if you just $result = $cfg->get('flag'); the $result is null, this follow the pattern used in the PSR-6 and PSR-16

I wan to know more about Scalar-to-array coercion and No delimiter escaping;, provide me unit tests where you can verify your concern and I can see better if it was intende or not.

Co-authored-by: overclokk <4604932+overclokk@users.noreply.github.com>
Copy link
Author

Copilot AI commented Jan 29, 2026

@copilot
has() treats stored null as “missing,” it is correct, in fact if you just $result = $cfg->get('flag'); the $result is null, this follow the pattern used in the PSR-6 and PSR-16

...

Added unit coverage to show the behaviors: (1) scalar-to-array coercion leaves the original scalar intact but nested access yields the default; (2) keys containing the delimiter require the array-path form to be retrieved, while dot-notation treats it as nesting. Commit be07163

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