Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused code from Process #52399

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

ausi
Copy link
Contributor

@ausi ausi commented Oct 31, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

While trying to work on #43162 (comment) I noticed that the private member useFileHandles is not used for anything anymore.
It is always set in the constructor to '\\' === \DIRECTORY_SEPARATOR so it basically has a constant value and its only usage is in an elseif after a check to the very same constant value: if ('\\' === \DIRECTORY_SEPARATOR).
Therefore this code does nothing and can be removed.

@carsonbot carsonbot added this to the 6.3 milestone Oct 31, 2023
@carsonbot carsonbot changed the title Remove dead code from Process Remove unused code from Process Oct 31, 2023
@GromNaN
Copy link
Member

GromNaN commented Nov 1, 2023

I don't know this particular part of the code but usually this kind of comparison DIRECTORY_SEPARATOR is used to detect windows systems. As the value is different on Windows and Unix systems.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Ok, this condition is actually checked in the previous if.

@xabbuh xabbuh modified the milestones: 6.3, 6.4 Nov 1, 2023
@@ -322,7 +320,7 @@ public function start(callable $callback = null, array $env = [])

if ('\\' === \DIRECTORY_SEPARATOR) {
$commandline = $this->prepareWindowsCommandLine($commandline, $env);
} elseif (!$this->useFileHandles && $this->isSigchildEnabled()) {
} elseif ($this->isSigchildEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} elseif ($this->isSigchildEnabled()) {
} elseif ('\\' !== \DIRECTORY_SEPARATOR && $this->isSigchildEnabled()) {

Copy link
Contributor

Choose a reason for hiding this comment

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

The elseif will only be considered if if ('\\' === \DIRECTORY_SEPARATOR) (2 lines before) doesn't hit. Basically it will be checked twice but vice versa, as suggested change. 🤔

@nicolas-grekas nicolas-grekas changed the base branch from 6.3 to 6.4 November 1, 2023 12:39
@nicolas-grekas
Copy link
Member

Thank you @ausi.

@nicolas-grekas nicolas-grekas merged commit d7c5c47 into symfony:6.4 Nov 1, 2023
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants