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

BUGFIX: Make php binary setting check use realpath #2032

Merged
merged 2 commits into from Jul 6, 2020

Conversation

albe
Copy link
Member

@albe albe commented Jun 19, 2020

The check if the current executing php binary path as set in PHP_BINARY matches the given setting in phpBinaryPathAndFilename was case-sensitive. This check is only done when the setting is specified and should guarantee that the path specified refers to the same binary as the one executing the current script. On Windows, the filesystem is case-insensitive by default though (Windows 10 has options to make it case-sensitive) and the chance that someone accidentially gets the casing of the path wrong on windows is (arguably) much more likely than someone having different versions of php installed on a case-sensitive filesystem in pathes /foo/PHP/php and /foo/php/php respectively.
Even if that would be the case, if the two versions do not match the check at ensureWebSubrequestsUseCurrentlyRunningPhpVersion would still throw an error with an even more specific message.

So this will prevent irritating errors on Windows in the form of
image

@albe albe added the Bug label Jun 19, 2020
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

I think the message is crystal-clear - it shows you two differently-cased paths. Is it really needed to automatically work around that? On a case-sensitive FS the check would now pass even when the case is wrong, so… I dare to object. Also since the number of installations on case-sensitive systems is way higher, I'd think.

@albe
Copy link
Member Author

albe commented Jun 29, 2020

On a case-sensitive FS the check would now pass even when the case is wrong, so…

Correct, but:

Even if that would be the case, if the two versions do not match the check at ensureWebSubrequestsUseCurrentlyRunningPhpVersion would still throw an error with an even more specific message.

So the change (for case-sensitive FS) in behavior would be:

  • if the two paths have different PHP versions the error message talks about version difference rather than path name case difference
  • if the two paths have same PHP versions, no error is thrown any more

The latter should be fine IMO, because what we actually need is same PHP version to guarantee consistent behaviour?

Edit: That check obviously is only for Web requests, while the check here in question is for CLI requests.

@bwaidelich
Copy link
Member

I'm a bit indifferent to be honest, but since my review was requested: I'm with Karsten on this one, if you configure the path just consider the case whether it matters or not.
Relying on a subsequent more strict check seems "dangerous" to me since the setting could be used elsewhere..

When we decide to fix this, I'd vote for making the exception a bit clearer (or even detect this case and explain it, i.e. "the path differs only in casing which might not be a problem on the current file system but should be fixed anyways blah blah")

@albe
Copy link
Member Author

albe commented Jun 29, 2020

if you configure the path just consider the case whether it matters or not.

Yeah, though at this point the issue is on windows this specifically basically never matters, but suddenly appears to (though in reality it still doesn't).

Relying on a subsequent more strict check seems "dangerous" to me since the setting could be used elsewhere

It's not about the setting, just about the check :) But as noticed above in the edit, the "subsequent" check is conditionally orthogonal to the one in question. I'll take another look, why we don't even check the PHP version for CLI requests, but only the PHP_BINARY path. I think it has to do with the fact we actively use PHP_BINARY at some place.

Edit: Related to #1643

Okay, so to sum up the edge-case this is about:

  • user has C:\php\php.exe configured as phpBinaryPathAndFilename
  • the "correct" path is C:\PHP\php.exe, but windows does not care
  • error is thrown though technically it would not be needed (on windows)
  • on *NIX the same case would be an issue IFF /foo/PHP/php and /foo/php/php are two different version binaries
  • the check for PHP_BINARY is made, so that phpBinaryPathAndFilename can point to a wrapper script

The last point brings me to the observation, that the check tries to execute $phpBinaryPathAndFilename -r "echo PHP_BINARY;", which should IMO resolve to the "correct" path. But apparently it doesn't.
Verification (executing php from the "wrong" uppercase path in my case):

PS C:\> C:\Dev\xampp-7.3\php\php.exe -r "echo PHP_BINARY;"
C:\Dev\xampp-7.3\php\php.exe
PS C:\> C:\DEV\XAMPP-7.3\PHP\php.exe -r "echo PHP_BINARY;"
C:\DEV\XAMPP-7.3\PHP\php.exe
PS C:\> C:\DEV\XAMPP-7.3\PHP\php.exe -r "echo realpath(PHP_BINARY);"
C:\Dev\xampp-7.3\php\php.exe

So I have a probably better solution: use realpath(PHP_BINARY) for the check (and maybe also the usage to build the subprocess command?)

This avoids configured path case issues on windows systems.
@albe albe changed the title BUGFIX: Make php binary setting check case-insensitive BUGFIX: Make php binary setting check use realpath Jun 29, 2020
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

🙄 oh, PHP…

@albe albe merged commit e3f09c9 into 5.3 Jul 6, 2020
@albe albe deleted the albe-phpbinary-check-case branch July 6, 2020 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants