[armanist]

Closes #467

Summary by CodeRabbit

• Refactor

  • Improved dependency injection container architecture by introducing a dedicated registry component. The container's constructor now accepts an optional registry parameter for enhanced flexibility and testability.

• Tests

  • Added comprehensive test coverage for the new registry component, validating dependency registration, resolution, and error handling.

[coderabbitai]

Warning

Rate limit exceeded

@armanist has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 34 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 34 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e96124ae-12f2-49fc-9576-9f99dd287f23

📥 Commits

Reviewing files that changed from the base of the PR and between 616f77a and 7461d5d.

📒 Files selected for processing (2)

• src/Di/DiRegistry.php
• tests/Unit/Di/DiRegistryTest.php

📝 Walkthrough

Walkthrough

A new DiRegistry class is introduced to manage dependency bindings, while DiContainer is refactored to delegate all registration and lookup operations to this registry instance. This separates binding definitions from runtime resolution logic while maintaining backward compatibility with the existing DiContainer API.

Changes

Cohort / File(s)	Summary
Registry Implementation src/Di/DiRegistry.php	New class that maintains abstract-to-concrete dependency bindings with methods for registration, lookup, validation, and clearing. Includes error handling for duplicate registrations, non-instantiable classes, and invalid abstracts.
Container Refactoring src/Di/DiContainer.php	Modified to accept an optional DiRegistry instance in constructor and delegate all binding operations (registerDependencies, register, isRegistered) to the registry. Resolution logic updated to retrieve concrete mappings via registry.
Registry Tests tests/Unit/Di/DiRegistryTest.php	Comprehensive test suite covering single and bulk registration, registration status checks, concrete mapping retrieval, error cases (duplicate, non-existent class, invalid abstract, unregistered lookup), and reset functionality.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Suggested labels

enhancement

Suggested reviewers

• andrey-smaelov

Poem

🐰 A rabbit hops through bound dependencies clean,
Separating what's stored from what's seen,
Registry holds bindings, Container runs fast,
Responsibilities split, concerns unsurpassed! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 47.62% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: splitting DI container responsibilities into a new DiRegistry class.
Linked Issues check	✅ Passed	All coding objectives from issue #467 are met: DiRegistry class created with required methods, DiContainer delegates binding logic to registry, backward compatibility maintained, and comprehensive tests added.
Out of Scope Changes check	✅ Passed	All changes are directly within scope: DiRegistry class, DiContainer updates to use registry, and focused DiRegistry tests. No changes to App, AppContext, or Di facade behavior.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.48%. Comparing base (fdfede2) to head (7461d5d).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #468      +/-   ##
============================================
+ Coverage     82.44%   82.48%   +0.03%     
- Complexity     2894     2902       +8     
============================================
  Files           250      251       +1     
  Lines          7798     7814      +16     
============================================
+ Hits           6429     6445      +16     
  Misses         1369     1369              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Di/DiRegistry.php`:
- Around line 40-46: The bulk binding method registerDependencies currently
ignores duplicates by checking isRegistered and skipping, but register() throws
on duplicates; change registerDependencies (method name: registerDependencies)
to not silently skip duplicates—either remove the isRegistered check and call
register($concrete, $abstract) for every entry so register() will throw on
duplicates, or explicitly detect duplicates and throw a clear exception (include
$abstract in the message) before calling register; update any tests accordingly
so duplicate bulk bindings surface the same error as single register() calls.
- Around line 61-63: Replace the current class existence check with an
instantiability check: after verifying class_exists($concrete) in the
registration code, instantiate a ReflectionClass for $concrete and call
ReflectionClass::isInstantiable(); if it returns false, throw
DiException::dependencyNotInstantiable($concrete). This ensures abstract classes
or other non-instantiable types are rejected at registration (refer to the
$concrete variable, the registration method where it’s checked,
DiException::dependencyNotInstantiable, and ReflectionClass::isInstantiable).

In `@tests/Unit/Di/DiRegistryTest.php`:
- Around line 49-55: Add a new unit test that covers an existing but
non-instantiable class (e.g., an abstract class) so the registry enforces
dependencyNotInstantiable; create a test method (e.g.,
testAttemptingToRegisterAbstractClass) that defines or references an abstract
class, calls $this->expectException(DiException::class) and
$this->expectExceptionMessage('The dependency `AbstractClass` is not
instantiable.'), then calls $this->registry->register('AbstractClass') to assert
registry->register triggers the same DiException path as the non-existent class
case.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c9afc147-ad66-4a22-98eb-eb06b44a3411

📥 Commits

Reviewing files that changed from the base of the PR and between fdfede2 and 616f77a.

📒 Files selected for processing (3)

• src/Di/DiContainer.php
• src/Di/DiRegistry.php
• tests/Unit/Di/DiRegistryTest.php

[armanist]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. What shall we delve into next?

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".