Skip to content

Fix delegate lookup in Container::has#402

Merged
samdark merged 4 commits into
masterfrom
fix-container-has-delegates
Jun 5, 2026
Merged

Fix delegate lookup in Container::has#402
samdark merged 4 commits into
masterfrom
fix-container-has-delegates

Conversation

@samdark

@samdark samdark commented Jun 2, 2026

Copy link
Copy Markdown
Member

Summary

  • make Container::has() respect configured delegate containers
  • avoid the no-delegate miss-path overhead by guarding delegate lookup with a hasDelegates flag
  • add regression coverage for delegate-only entries

Fixes #401.

Tests

  • vendor/bin/phpunit --testdox
  • vendor/bin/phpbench run tests/Benchmark/ContainerMethodHasBench.php --report=default --group=has

Benchmark note

With xdebug on and opcache off, the guarded fix keeps the no-delegate nonexistent lookup path at baseline: about 401 us for 200 nonexistent IDs versus about 405 us before the change. The unguarded variant measured about 463 us.

Copilot AI review requested due to automatic review settings June 2, 2026 20:53
@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (59579ea) to head (642a4a1).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##              master      #402   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       184       188    +4     
===========================================
  Files             11        11           
  Lines            500       509    +9     
===========================================
+ Hits             500       509    +9     

☔ View full report in Codecov by Harness.
📢 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes Yiisoft\Di\Container::has() so it respects configured delegate containers (PSR-11 behavior), and adds regression coverage for “delegate-only” entries while avoiding extra overhead when no delegates are configured.

Changes:

  • Update Container::has() to consult delegates (guarded by a new hasDelegates flag).
  • Track whether any delegates are configured during setDelegates().
  • Add a unit test ensuring has() and get() work for entries provided only by a delegate container.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Container.php Makes has() consult delegate containers and adds a flag to avoid delegate lookup overhead when none are configured.
tests/Unit/ContainerTest.php Adds regression test for delegate-provided entries being discoverable via has().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Container.php Outdated
Comment thread tests/Unit/ContainerTest.php Outdated
@samdark samdark force-pushed the fix-container-has-delegates branch 3 times, most recently from 2942f68 to 2e93548 Compare June 2, 2026 21:42
@samdark samdark requested a review from Copilot June 2, 2026 21:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/Container.php Outdated
Comment thread tests/Unit/ContainerTest.php
@samdark samdark force-pushed the fix-container-has-delegates branch 2 times, most recently from b3c4a46 to 8249691 Compare June 2, 2026 22:00
@samdark samdark force-pushed the fix-container-has-delegates branch from 8249691 to fa0b795 Compare June 2, 2026 22:05
@samdark samdark requested a review from vjik June 2, 2026 22:09
Comment thread src/Container.php Outdated

@vjik vjik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add line to changelog

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread CHANGELOG.md Outdated
@samdark samdark merged commit f99d96d into master Jun 5, 2026
24 checks passed
@samdark samdark deleted the fix-container-has-delegates branch June 5, 2026 18:19
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.

Container::has() doesn't respect delegates, violating PSR-11

3 participants