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: Flow CLI command warns of mismatching php version #1391

Merged

Conversation

Projects
None yet
6 participants
@beheist
Copy link
Member

commented Sep 7, 2018

If Flow builds a PHP command for a subrequest, it uses the system default if nothing else is configured. With this change, we avoid Flow executing that request if it isn't explicitly configured to use that same PHP version internally too. This should avoid some errors especially in shared hosting scenarios for less experienced users.

Bastian Heist
@beheist

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

This is that it looks like in the console:
image

return $command;
}
/**
* @param $command

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 7, 2018

Member

missing type hint

protected static function ensureCurrentPhpVersionMatchesConfiguredVersion($command)
{
$runningPHPVersion = phpversion() . '-' . PHP_SAPI;
exec($command . ' -r "echo phpversion() . \'-\' . PHP_SAPI;"', $output, $result);

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 7, 2018

Member

This would run an additional exec each time. Didn't you mean we could check against the PHP_BINARY to avoid that?

This comment has been minimized.

Copy link
@beheist

beheist Sep 7, 2018

Author Member

Yes, I can do that too. We can't compare the versions in that case, but that's not necessary I guess.

$runningPHPVersion = phpversion() . '-' . PHP_SAPI;
exec($command . ' -r "echo phpversion() . \'-\' . PHP_SAPI;"', $output, $result);
if ($result === 0) {

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 7, 2018

Member

an early return would improve readability IMO:

if ($result !== 0) {
    return;
}

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 7, 2018

Member

But then again - should we ignore that error silently?

This comment has been minimized.

Copy link
@beheist

beheist Sep 7, 2018

Author Member

We check that elsewhere in the script:
https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Core/Booting/Scripts.php#L360-L375

so I didn't think it's necessary to duplicate that logic there.

Bastian Heist
@beheist

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

I changed this to compare binary paths instead of versions, and adapted the error message slightly.
This check will only run if Flow executes a subrequest, but that's the only place where it matters, so that is probably OK.
image

return;
}
if (strcmp(PHP_BINARY, $configuredPhpBinaryPathAndFilename) !== 0) {

This comment has been minimized.

Copy link
@albe

albe Sep 7, 2018

Member

I'll have to check if this works on windows.

PS: OTOH if that would work reliably, we wouldn't have to have the configuration setting in the first place.

This comment has been minimized.

Copy link
@beheist

beheist Sep 9, 2018

Author Member

Hah, you're right. I wasn't even thinking about that. Additionally I need to check what happens if proxies get compiled via a web request instead of a CLI request. Setting this to WIP for now.

This comment has been minimized.

Copy link
@beheist

beheist Sep 10, 2018

Author Member

So, yeah - this is what I thought would happen and probably also the entire reason we have that config setting.
image

I see two ways forward here:

  1. Do the check only for CLI requests. This would make the error message disappear, but again open the possibility for hard-to-detect configuration errors
  2. Do the check as-is for CLI requests, and do the check like before for non-CLI (run exec(), checking the PHP version of the configured PHP-CLI, and compare the actual version number with the running one).

I prefer the second option, as it makes config errors impossible, gives the user a precise and actionable error message, and has a minimal performance impact (delays the building of proxies by one exec(), but since proxy building takes a few secs at minimum anyway, this should be hardly noticeable).

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 10, 2018

Member

I'm not so much concerned about performance. but option 2 spawns an additional process for every sub request. For packages that use that mechanism extensively (JobQueue, EventSourcing, ...) that doubles the amount of processes. I don't know how problematic that is, but we should evaluate that IMO.

This comment has been minimized.

Copy link
@beheist

beheist Sep 10, 2018

Author Member

But the process is only spawned if proxies have not been built yet - once proxies are compiled, we don't reach this section of code. Is this still a problem in this case (I don't know how the other packages work, so..)?

This comment has been minimized.

Copy link
@beheist

beheist Sep 10, 2018

Author Member

I've pushed my WIP now, so we can discuss further based on that :)

This comment has been minimized.

Copy link
@beheist

beheist Sep 10, 2018

Author Member

WWW Error message looks like this now:
image

CLI is still the same.

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 10, 2018

Member

Ah OK, sorry, I only watched with one eye :)
The error message is much clearer, great improvement!

This comment has been minimized.

Copy link
@beheist

beheist Sep 10, 2018

Author Member

OK, removing WIP. Final reviews please :)

This comment has been minimized.

Copy link
@albe

albe Dec 4, 2018

Member

The two-lane way seems to work good. Just need to fix the strict version constraint.

@beheist beheist changed the title Fix flow booting with differing php versions [WIP] Fix flow booting with differing php versions Sep 9, 2018

Bastian Heist
TASK: Differentiate between Web and CLI requests and throw a detailed…
… exception if the PHP version for subrequests has not been configured correctly in each case.

@beheist beheist force-pushed the beheist:dev-bh-fix-flow-booting-with-differing-php-versions branch from 43d1f2d to 3679888 Sep 10, 2018

@beheist beheist changed the title [WIP] Fix flow booting with differing php versions Fix flow booting with differing php versions Sep 10, 2018

$configuredPhpBinaryPathAndFilename = realpath($phpBinaryPathAndFilename);
// if the configured PHP binary is empty here, the file does not exist. We ignore that here because it is checked later in the script.
if (strlen($configuredPhpBinaryPathAndFilename) === 0) {

This comment has been minimized.

Copy link
@helhum

helhum Sep 12, 2018

Contributor

realpath will return false if the configured path/file does not exist.
This means you are calling a string function on a boolean in that case. Maybe check the boolean case first?

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 13, 2018

Member

Good catch

This comment has been minimized.

Copy link
@helhum

helhum Sep 13, 2018

Contributor

Oh, and while reading setup package code, if open_basedir is configured, it may well be that the php binary path is not included, thus regular file functions would not work on that path.

So something like this would be required to catch this case.

return;
}
if (strcmp(PHP_BINARY, $configuredPhpBinaryPathAndFilename) !== 0) {

This comment has been minimized.

Copy link
@helhum

helhum Sep 12, 2018

Contributor

Two questions here:

  1. Why not directly always using PHP_BINARY for the sub process instead of relying on some config. AFAIK this also works reliably on Windows
  2. Isn't PHP_BINARY === $configuredPhpBinaryPathAndFilename easier to grasp than using strcmp?

This comment has been minimized.

Copy link
@kitsunet

kitsunet Sep 12, 2018

Member

From what I remember the PHP_BINARY was unreliable on shared environments with multiple PHP versions or something like that? I don't really remember the case.

This comment has been minimized.

Copy link
@albe

albe Dec 4, 2018

Member

PHP_BINARY is not reliable on Windows from Web. It will just point to the process that launched php (e.g. apache/nginx).

This comment has been minimized.

Copy link
@helhum

helhum Dec 4, 2018

Contributor

PHP_BINARY is never to rely on for web requests. It is reliable on cli and always points to the path of the currently in use PHP binary. This whole code is only executed on cli. So why not just rely on PHP_BINARY, because it would just work.

Like that:

if (PHP_SAPI === 'cli') {
   return PHP_BINARY
}

return $settings['configuredPHPBinaryForWebRequests'];

This works reliably in practice across operating systems for me for years on a fairly large user base.

The only complaint I received was from a user that called the script with additional options for the php binary like so: php -derror_reporting=0, which is not easily solvable, except such options are configurable as well.

This comment has been minimized.

Copy link
@albe

albe Dec 4, 2018

Member

But this method is not responsible for finding the php binary, but rather check if the found (configured) php binary matches the one that this command is currently run with to detect configuration errors :) Also, we probably shouldn't use PHP_BINARY in the one case (CLI) and rely on a configuration setting (phpBinaryPathAndFilename) in the other (Web), even if that would make it work in at least one case reliably.

This comment has been minimized.

Copy link
@helhum

helhum Dec 4, 2018

Contributor

even if that would make it work in at least one case reliably

If there is a chance to make it more reliable for one case, why not pick this low hanging fruit? Just to have it consistently unreliable for both cases?

But this method is not responsible for finding the php binary, but rather check if the found (configured) php binary matches the one that this command is currently run with to detect configuration errors

I know, I wanted to point out that the whole check could be made obsolete for CLI.

}
$runningPHPVersion = phpversion();
$configuredPHPVersion = $output[0];
if ($runningPHPVersion !== $configuredPHPVersion) {

This comment has been minimized.

Copy link
@helhum

helhum Sep 12, 2018

Contributor

This could be a quite intrusive limitations for some systems/ hosting environments.
Since PHP for web requests and PHP on cli are completely different binaries, in theory versions can also differ. E.g. PHP 7.1.3 for web requests and PHP 7.1.2 on CLI

With this change, such constellation would throw an exception. Right?

This comment has been minimized.

Copy link
@kitsunet

kitsunet Sep 12, 2018

Member

I agree, I don't think we need to be that strict. I would rather loosen it to same minor or something. Ideally we would go into the direction of https://github.com/kitsunet/flownative-webcompiler at some point to avoid the CLI thing altogether.

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 13, 2018

Member

I would rather loosen it to same minor or something

Yes, that's what we did in the Setup package. It's not a beauty, but it seems to work:
https://github.com/neos/setup/blob/master/Classes/Core/RequestHandler.php#L173-L271

Ideally we would go into the direction of https://github.com/kitsunet/flownative-webcompiler

It would be great if compilation worked without additional requests (I hope this works memory wise, too). But CLI "sub requests" are useful anyways (i.e. for "poor mans" job queue, batched CLI commands, ...). So +1 for making this more stable anyways

This comment has been minimized.

Copy link
@albe

albe Dec 4, 2018

Member

Yes, definitely needs to be less strict. Minor version match is enough.

Show resolved Hide resolved Neos.Flow/Classes/Core/Booting/Scripts.php
$configuredPhpBinaryPathAndFilename = realpath($phpBinaryPathAndFilename);
// if the configured PHP binary is empty here, the file does not exist. We ignore that here because it is checked later in the script.
if (strlen($configuredPhpBinaryPathAndFilename) === 0) {

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 13, 2018

Member

Good catch

}
$runningPHPVersion = phpversion();
$configuredPHPVersion = $output[0];
if ($runningPHPVersion !== $configuredPHPVersion) {

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Sep 13, 2018

Member

I would rather loosen it to same minor or something

Yes, that's what we did in the Setup package. It's not a beauty, but it seems to work:
https://github.com/neos/setup/blob/master/Classes/Core/RequestHandler.php#L173-L271

Ideally we would go into the direction of https://github.com/kitsunet/flownative-webcompiler

It would be great if compilation worked without additional requests (I hope this works memory wise, too). But CLI "sub requests" are useful anyways (i.e. for "poor mans" job queue, batched CLI commands, ...). So +1 for making this more stable anyways

@jonnitto jonnitto requested review from kitsunet, albe and bwaidelich Nov 25, 2018

albe added some commits Dec 4, 2018

@albe

albe approved these changes Dec 4, 2018

Copy link
Member

left a comment

I guess there could still be a lot improved (especially with bringing this more in line with https://github.com/neos/setup/blob/master/Classes/Core/RequestHandler.php#L145 ff. but as is, this now provides helpful information when the configured php binary does not match the php version that the current request/command is run with.

@albe

albe approved these changes Dec 4, 2018

@albe albe removed the I: Needs feedback label Apr 20, 2019

@albe albe changed the title Fix flow booting with differing php versions BUGFIX: Flow CLI command warns of mismatching php version Apr 20, 2019

@albe

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

@bwaidelich @kitsunet Can one of you approve this thing and merge?

@bwaidelich

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

If this is a bugfix it should go to the lowest supported branch, no?
And if it's considered too risky it could go to master..

Apart from that: It looks good to me by reading but I can't test this really (at least not on Windows) so I don't dare to merge this

@kdambekalns
Copy link
Member

left a comment

Seems this still has some open questions…

Show resolved Hide resolved Neos.Flow/Classes/Core/Booting/Scripts.php Outdated
@albe

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

If this is a bugfix it should go to the lowest supported branch, no?

Correct, which is 4.3 as of the time now :) Anything lower is security fixes only.

@bwaidelich

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

ah right.. Flow != Neos :)

@albe

This comment has been minimized.

Copy link
Member

commented May 16, 2019

So, anything left keeping this from getting in? I'd really like to get this merged as is to improve the situation. We can still discuss on a better way of solving things in a new issue if needed.

@albe

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Ok, I'll take responsibility for this one, if it messes things up. I'll merge and provide follow-up fixes where necessary.

@albe albe merged commit ed39e54 into neos:4.3 May 29, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kitsunet

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

The import of use Neos\Flow\Exception; in this change breaks things because the SubProcessException is no longer found, masking an error in the compile step with a PHP fatal error...

See \Neos\Flow\Core\Booting\Scripts::executeCommand where the Exception with code 1355480641 is thrown.

kdambekalns added a commit to kdambekalns/flow-development-collection that referenced this pull request Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.