Matched DDEV and Lando stacks by their env marker, not a container probe.#105
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR changes stack selection so DDEV and Lando activate from their markers before the generic container fallback, updates ChangesStack detection and docs
Sequence Diagram(s)sequenceDiagram
participant Environment
participant Ddev
participant Lando
participant Container
Environment->>Ddev: active()
alt IS_DDEV_PROJECT marker is present
Ddev-->>Environment: true
else
Environment->>Lando: active()
alt LANDO_INFO marker is present
Lando-->>Environment: true
else
Environment->>Container: active()
alt container probe matches
Container-->>Environment: true
else
Environment-->>Environment: Native fallback
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
🚀 Performance comparison (informational)A tracking signal, not a pass/fail gate: microsecond benchmarks land on a different shared CI runner each run, so percentages vs the committed baseline shift run-to-run regardless of the code.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 19
Lines 264 262 -2
=========================================
- Hits 264 262 -2 ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/README.md`:
- Line 42: The runtime-cost description is too narrow for the generic container
probe; update the prose to match src/Stacks/Container.php by stating that the
unlabelled-container path is a fallback chain, not a single filesystem check.
Reference the Container class behavior explicitly: it checks environment markers
first, then probes /.dockerenv and /.dockerinit, and finally inspects
/proc/1/cgroup when earlier checks miss. Keep the wording aligned with the
actual detection flow in the benchmark README.
In `@src/Environment.php`:
- Around line 447-454: Add an Environment-level regression test for marker-only
stack selection in getActiveStack(). The current EnvironmentTest coverage only
verifies generic container detection and DOCKER plus IS_DDEV_PROJECT, so it does
not exercise the reordered collectStacks()/active() path end-to-end for
marker-only activation. Update EnvironmentTest to include assertions for
IS_DDEV_PROJECT-only and LANDO_INFO-only cases, using
Environment::getActiveStack() to confirm the Ddev and Lando stacks are selected
correctly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8ef1b637-c990-43d4-ac45-f89320054526
📒 Files selected for processing (7)
README.mdbenchmarks/README.mdsrc/Environment.phpsrc/Stacks/Container.phptests/Stacks/ContainerTest.phptests/Stacks/DdevTest.phptests/Stacks/LandoTest.php
…marker-only stack tests.
Contributor
🚀 Performance comparison (informational)A tracking signal, not a pass/fail gate: microsecond benchmarks land on a different shared CI runner each run, so percentages vs the committed baseline shift run-to-run regardless of the code.
|
AlexSkrypnyk
added
the
Needs review
Contributor
🚀 Performance comparison (informational)A tracking signal, not a pass/fail gate: microsecond benchmarks land on a different shared CI runner each run, so percentages vs the committed baseline shift run-to-run regardless of the code.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/README.md`:
- Line 50: The first optimization note in the benchmarks README is awkwardly
phrased; rewrite the sentence describing DDEV and Lando so it reads clearly and
grammatically while keeping the same meaning. Update the wording around the
container-family stack detection note in the benchmarks section, preserving the
explanation that DDEV and Lando are identified by their definitive markers and
do not need to probe.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19dd199f-c398-464a-b215-b39c0a03ce56
📒 Files selected for processing (2)
benchmarks/README.mdsrc/Stacks/Container.php
Contributor
🚀 Performance comparison (informational)A tracking signal, not a pass/fail gate: microsecond benchmarks land on a different shared CI runner each run, so percentages vs the committed baseline shift run-to-run regardless of the code.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DdevandLandostacks now match purely on the definitive environment marker their tooling sets (IS_DDEV_PROJECT,LANDO_INFO), removing a guard that required a generic container probe before the marker was even consulted. The genericContainerandNativefallbacks are moved out of theSTACKSconstant and appended last incollectStacks(), so any specific stack - built-in, user-supplied, or a subclass - is always matched ahead of the generic it extends. Together these changes fix a latent false-negative on container runtimes that the generic probe cannot recognise, where detection would otherwise fall through to the native host.Changes
Stack detection logic (
src/Environment.php)if ($stack instanceof Container && !$stack->isContainer())guard fromgetActiveStack(); each stack's ownactive()now decides without a preceding container probe.Container::classout of theSTACKSconstant into the generic tail appended bycollectStacks(), so specific stacks always run first;Nativeremains the final always-true fallback.Containerdoc comment (src/Stacks/Container.php)isContainer()doc comment to be caller-agnostic; it no longer names DDEV/Lando as callers, since they no longer probe.Tests (
tests/Stacks/DdevTest.php,tests/Stacks/LandoTest.php,tests/Stacks/ContainerTest.php)DdevTestandLandoTestmarker-alone cases now assertTRUE(wasFALSEunder the old guard).ContainerTestcomment updated to reflect that a specific marker activates that specific stack, not the generic container.Docs (
README.md,benchmarks/README.md)README.mdStacks section updated: DDEV and Lando match by their tool marker;Containermatches by probing.benchmarks/README.md: added a "Cost in context" section explaining that detection is a tiny fraction of a page request and that OPcache amortises structural overhead to near-zero in production; updated Optimization notes item 1 to reflect the two-part fix; corrected a stale section cross-reference and a~41 μsfigure now~28 μs.Before / After
Summary by CodeRabbit