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

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

Open
wants to merge 3 commits into
base: 6.3
Choose a base branch
from

Conversation

TravisCarden
Copy link
Contributor

@TravisCarden TravisCarden commented Nov 22, 2023

...except (probably) on Windows.

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
License MIT

ExecutableFinder::addSuffix() currently has no effect on non-Windows systems because ExecutableFinder::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, but ExecutableFinder::find() returned a different temp path than sys_get_temp_dir(), rendering path comparison impossible. So I added a few fixture files directly to the repo.

@carsonbot carsonbot added this to the 6.4 milestone Nov 22, 2023
@TravisCarden TravisCarden force-pushed the feature/ExecutableFinder-find-with-added-alias branch 2 times, most recently from 59697bd to 1187d16 Compare November 22, 2023 06:10
@OskarStark OskarStark changed the title [Process] ExecutableFinder::addSuffix() has no effect [Process] ExecutableFinder::addSuffix() has no effect Nov 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?

…leFinder::addSuffix()

It's not clear from the current docblock that the suffix should have a dot at the beginning.
…nt\Process\ExecutableFinder::find()

Proves that added-alias-handling is broken.
…utableFinder::find()

Makes the failing tests pass.
@TravisCarden
Copy link
Contributor Author

@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? It does already exist in both. I've rebased onto 6.3. Is that what you want? Or do you want me to go all the way back to 5.4? (I thought I understood the bugfix policy on this, but maybe I don't.)

@stof
Copy link
Member

stof commented Nov 27, 2023

This option also has no effect on Windows when the PATH_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

stof commented Nov 27, 2023

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
Contributor Author

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
*/
public function addSuffix(string $suffix)

In other words, unless users are expected to search the code before using any 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

stof commented Nov 27, 2023

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

@stof
Copy link
Member

stof commented Nov 27, 2023

Given that suffixes are totally ignored on non-Windows systems and are ignored on Windows systems that have the PATH_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
Contributor Author

if you need to find an executable named composer.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 find composer.phar either anyway.

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

$finder = new ExecutableFinder();
$finder->addSuffix('.phar');
$path = $finder->find('composer2');
var_dump($path);

// string(28) "/usr/local/bin/composer2.phar"

$process = new Process([$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.

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

5 participants