Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Process]ExecutableFinder::addSuffix() has no effect#52679

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

Conversation

@TravisCarden
Copy link
Contributor

@TravisCardenTravisCarden commentedNov 22, 2023
edited by fabpot
Loading

...except (probably) on Windows.

QA
Branch?7.2
Bug fix?yes
New feature?no
Deprecations?no
LicenseMIT

ExecutableFinder::addSuffix() currently has no effect on non-Windows systems becauseExecutableFinder::find() altogether ignores added suffixes on them. This PR adds tests that prove that it's broken and then fixes it. It clarifies a related docblock along the way.

Note: I tried to follow the pattern in similar tests of creating fixture files in the temp directory, butExecutableFinder::find() returned a different temp path thansys_get_temp_dir(), rendering path comparison impossible. So I added a few fixture files directly to the repo.

@carsonbotcarsonbot added this to the6.4 milestoneNov 22, 2023
@TravisCardenTravisCardenforce-pushed thefeature/ExecutableFinder-find-with-added-alias branch 2 times, most recently from59697bd to1187d16CompareNovember 22, 2023 06:10
@OskarStarkOskarStark changed the title[Process] ExecutableFinder::addSuffix() has no effect[Process]ExecutableFinder::addSuffix() has no effectNov 23, 2023
@nicolas-grekas
Copy link
Member

Can you please check if this bugfix shouldn't be applied to either 5.4 or 6.3 and rebase/retarget if yes?

@TravisCarden
Copy link
ContributorAuthor

@nicolas-grekas Do you meant you want me to see if the bug already exists in 5.4 or 6.3 and rebase if so? Itdoes already exist in both. I've rebased onto 6.3. Is that what you want? Or do you want me to goall the way back to 5.4? (I thought I understood the bugfix policy on this, but maybe I don't.)

@stof
Copy link
Member

This option also has no effect on Windows when thePATH_EXT environment variable is provided.

As this option has been a no-op for other OS since the creation of the process component, I don't think we should change that as a bugfix.

@stof
Copy link
Member

Btw, what is your use case for using this on non-Windows systems ? AFAICT, only Windows automatically adds an implicit extension to the executable name when running it.

@TravisCarden
Copy link
ContributorAuthor

what is your use case for using this on non-Windows systems ?

I need to look for executables that may end in.phar--Composer, in this case.

AFAICT, only Windows automatically adds an implicit extension to the executable name when running it.

That is, in fact, the current behavior--and that's the problem. The in-code documentation gives no indication that the behavior is conditioned on OS, leaving no reason for a user to suspect otherwise:

/**
* Adds new possible suffix to check for executable.
*
* @return void
*/
publicfunctionaddSuffix(string$suffix)

In other words, unless users are expected to search the code before usingany and every function in the codebase to see whether it works to their OS, this doesn't behave as advertised. That seems like a bug to me.

@stof
Copy link
Member

@TravisCarden if you need to find an executable namedcomposer.phar, look for that. Because runningcomposer in a shell won't automatically findcomposer.phar either anyway.

@stof
Copy link
Member

Given that suffixes are totally ignored on non-Windows systems and are ignored on Windows systems that have thePATH_EXT environment variable (which is taken as the source of truth instead), I would rather deprecate those methods in PHP 7.1 than suddenly turning them into doing something.
the current behavior has been implemented in April 2011.

@TravisCarden
Copy link
ContributorAuthor

if you need to find an executable namedcomposer.phar, look for that.

I can't, because I'm writing a Packagist package, so I don't know that detail about my end users. Isn't that the point of suffixes?

Because running composer in a shell won't automatically findcomposer.phar either anyway.

Actually, it will, if I understand you. After applying this PR, and addingcomposer2.phar to avoid conflict with my global Composer install, it works for me from start to finish:

$finder =newExecutableFinder();$finder->addSuffix('.phar');$path =$finder->find('composer2');var_dump($path);// string(28) "/usr/local/bin/composer2.phar"$process =newProcess([$path,'--version']);$process->run();var_dump($process->getOutput());// string(43) "Composer version 2.6.5 2023-10-06 10:11:52"

I would rather deprecate those methods in PHP 7.1 than suddenly turning them into doing something.

Obviously, that's up to you. As I see it, that would make sense if you were talking about adding new behavior that was never claimed or advertised in the first place. But as I argued above, this is how it claimed to work in the first place, so this would just be making good on a promise it has always made but never kept.

@TravisCarden
Copy link
ContributorAuthor

Rebased,@nicolas-grekas.

I have my opinion as to whether this is a bug or not, but I don't really care about the semantics if this could be committed tosome version. If we can't commit it as-is, can someone tell me what it take?

@TravisCardenTravisCardenforce-pushed thefeature/ExecutableFinder-find-with-added-alias branch 2 times, most recently from4027764 to1651d3fCompareMay 15, 2024 22:20
@TravisCarden
Copy link
ContributorAuthor

It doesn't look like the CI failures are from my changes. Requesting re-review.

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

Looking at the CI failure, it does seem that tests are broken by this change:

1) Symfony\Component\Process\Tests\ExecutableFinderTest::testFindWithDefaultSuffixFailed asserting that null matches expected '/home/runner/work/symfony/symfony/src/Symfony/Component/Process/Tests/Fixtures/executable_with_a_default_suffix.bat'.

@TravisCarden
Copy link
ContributorAuthor

TravisCarden commentedJun 3, 2024
edited
Loading

Oh, of course. Because that test asserts on behavior that is now Windows-only. I've just removed it. I'll go ahead and rebase, while I'm at it.

@TravisCardenTravisCarden requested a review fromfabpotJune 3, 2024 13:51
@TravisCardenTravisCardenforce-pushed thefeature/ExecutableFinder-find-with-added-alias branch from18e4064 toa2ddf63CompareJune 3, 2024 14:00
@fabpotfabpotforce-pushed thefeature/ExecutableFinder-find-with-added-alias branch froma2ddf63 tod854b76CompareJune 4, 2024 06:32
@fabpot
Copy link
Member

Thank you@TravisCarden.

TravisCarden reacted with hooray emoji

@fabpotfabpot merged commit9ca598c intosymfony:7.2Jun 4, 2024
@TravisCardenTravisCarden deleted the feature/ExecutableFinder-find-with-added-alias branchJune 4, 2024 18:09
@fabpotfabpot mentioned this pull requestOct 27, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@xabbuhxabbuhxabbuh approved these changes

@lyrixxlyrixxAwaiting requested review from lyrixx

@ycerutoycerutoAwaiting requested review from yceruto

@kbondkbondAwaiting requested review from kbond

@chalasrchalasrAwaiting requested review from chalasr

@dunglasdunglasAwaiting requested review from dunglas

@OskarStarkOskarStarkAwaiting requested review from OskarStark

@jderussejderusseAwaiting requested review from jderusse

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees

No one assigned

Projects

None yet

Milestone

7.2

Development

Successfully merging this pull request may close these issues.

6 participants

@TravisCarden@nicolas-grekas@stof@fabpot@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp