Normalize temp dir in Given a directory step#329
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the temporary directory handling in GivenStepDefinitions.php to resolve macOS path normalization issues by incorporating realpath() and stripping the /private/var/ prefix. The review feedback recommends extending the path normalization to include /private/tmp/ to ensure consistency and refactoring the directory assignment logic for better readability.
There was a problem hiding this comment.
Pull request overview
This PR updates the Behat test contexts to better normalize temp directory paths (notably on macOS) so the Given ... directory step can safely validate and delete directories under the system temp directory even when paths differ due to /private prefixing.
Changes:
- Canonicalize
sys_get_temp_dir()viarealpath()before normalizing and comparing paths ingiven_a_specific_directory(). - Add additional macOS-specific normalization for the computed temp dir.
- Canonicalize the generated
RUN_DIRpath viarealpath()aftermkdir().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Context/GivenStepDefinitions.php |
Updates temp dir computation/normalization used for the safety check in the “Given a directory” step. |
src/Context/FeatureContext.php |
Canonicalizes the newly created RUN_DIR path using realpath(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $temp_dir = realpath( sys_get_temp_dir() ); | ||
| $temp_dir = $temp_dir ? Path::normalize( $temp_dir ) : Path::normalize( sys_get_temp_dir() ); | ||
| $dir = Path::normalize( $dir ); | ||
|
|
||
| // Also normalize temp dir for Mac OS X. | ||
| $temp_dir = preg_replace( '|^/private/var/|', '/var/', $temp_dir ); | ||
|
|
There was a problem hiding this comment.
realpath( sys_get_temp_dir() ) can resolve macOS temp paths to /private/tmp (e.g. when sys_get_temp_dir() is /tmp), but the current normalization only rewrites /private/var. This can make the safety check reject directories under /tmp because $dir may start with /tmp while $temp_dir starts with /private/tmp. Consider normalizing a leading /private/ prefix (not just /private/var) for both $dir and $temp_dir, or allowing both the realpath and non-realpath variants when checking the prefix.
| // Also check for temp dir prefixed with `/private` for Mac OS X. | ||
| if ( 0 !== strpos( $dir, $temp_dir ) && 0 !== strpos( $dir, "/private{$temp_dir}" ) ) { |
There was a problem hiding this comment.
The containment check uses strpos( $dir, $temp_dir ) === 0, which can incorrectly allow paths that merely share a prefix (e.g. temp dir /tmp and dir /tmp2/...). To avoid accidental deletion outside the intended temp dir, tighten this to require a path boundary (exact match or prefix followed by a directory separator after normalization).
| $temp_dir = realpath( sys_get_temp_dir() ); | ||
| $temp_dir = $temp_dir ? Path::normalize( $temp_dir ) : Path::normalize( sys_get_temp_dir() ); | ||
| $dir = Path::normalize( $dir ); | ||
|
|
||
| // Also normalize temp dir for Mac OS X. | ||
| $temp_dir = preg_replace( '|^/private/var/|', '/var/', $temp_dir ); | ||
|
|
There was a problem hiding this comment.
The updated temp-dir canonicalization is OS/filesystem-dependent (e.g. /tmp vs /private/tmp, /var vs /private/var). There are Behat scenarios covering the “Given a specific directory” step, but none appear to exercise these canonicalization differences; adding a scenario that derives both sys_get_temp_dir() and realpath(sys_get_temp_dir()) and verifies both path forms are accepted would help prevent regressions on macOS runners.
This fixes some failing Windows tests in language-command and potentially others using
Given a(n) ... directory.