Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Process] Return built-in cmd.exe commands directly in ExecutableFinder#58735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Seldaek commentedNov 2, 2024
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | Refs#58734 |
| License | MIT |
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(with minor CS change)
Thank you@Seldaek. |
81bffdf intosymfony:5.4Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| $finder = new ExecutableFinder(); | ||
| $this->assertSame('rmdir', $finder->find('RMDIR')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AppVeyor returnsRMDIR but notrmdir (see the failing test). Do we need to fix the expectation here or is there still some flaw in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the command names are case insensitive so I guess the implementation is fine but test expectations should be fixed to match the input's case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 thanks for the feedback (see#58748 then)