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

Subprocess failure on "upgrade:all" with PHP deprecation messages #751

Closed
mbrodala opened this issue Sep 7, 2018 · 12 comments
Closed

Subprocess failure on "upgrade:all" with PHP deprecation messages #751

mbrodala opened this issue Sep 7, 2018 · 12 comments
Labels

Comments

@mbrodala
Copy link
Contributor

mbrodala commented Sep 7, 2018

Bug Report

Current Behavior
If PHP shows deprecation messages (e.g. for usage of track_errors in php.ini) the upgrade:all command fails. This happens because the UpgradeHandling executes upgrade:subprocess and unserializes the response which in this case can contain messages from PHP in addition to a serialized response:

return @unserialize($this->commandDispatcher->executeCommand('upgrade:subprocess', [$command, serialize($arguments)]));

Expected behavior/output
The upgrade:all command should skip messages like these.

TYPO3 Console command executed

typo3cms upgrade:all

TYPO3 Console command output
output

Environment

  • TYPO3 Console version(s): 5.5.4
  • TYPO3 version(s): 8.7
  • Is your TYPO3 installation set up with Composer (Composer Mode): yes
  • OS: Ubuntu Linux

Possible Solution
The response from upgrade:subprocess should be sanitized before unserialization.

@helhum
Copy link
Member

helhum commented Sep 7, 2018

My first question to you would be: where is track_errors used? Is it in a third party extension?

@mbrodala
Copy link
Contributor Author

mbrodala commented Sep 7, 2018

It's a default php.ini option provided by PHP itself and is deprecated since PHP 7.2. This means if this option is used, even if set to Off the deprecation message is shown.

But the same can occur anytime for any other deprecation in PHP.

@helhum
Copy link
Member

helhum commented Sep 7, 2018

Since PHP outputs this startup error message early on, before any php script is triggered and it output this message to stdout not stderr, I see no way to fix this particular issue.

Other notices triggered during runtime are either muted by our own error handler or the one from TYPO3, so they won't appear in the output. That is at least my observation.

If you have a way to reproduce a failure with runtime error messages, please let me know.

@helhum
Copy link
Member

helhum commented Sep 7, 2018

I also have no other idea how to let a subprocess cleanly communicate to the parent process, except from the hacky approach to use stdout for that. If you know from other ways to achieve that, let me know as well.

@mbrodala
Copy link
Contributor Author

mbrodala commented Sep 7, 2018

A safe solution would be a separate file descriptor instead of stdout but I'm not sure Symfony Process supports this.

Aside from that the only option I see ATM is to drop everything from the subprocess output except of the last line which is supposed to contain the actual output:

  • Subprocess output:

    Deprecated: ...
    
    Deprecated: ...
    
    <serialized-string>
    
  • After sanitation:

    <serialized-string>
    

@helhum
Copy link
Member

helhum commented Sep 7, 2018

separate file descriptors are not supported by Symfony Process

Regarding sanitation: I have no clue how we should detect what should be kept and what should be discarded

@mbrodala
Copy link
Contributor Author

mbrodala commented Sep 7, 2018

Maybe I'm naive but basically like this:

$output = $this->commandDispatcher->....;
$sanitizedOutput = array_pop(explode("\n", $output));

return $sanitizedOutput;

@helhum
Copy link
Member

helhum commented Sep 7, 2018

your code would cover exactly one case where exactly one line is prepended to the output. I'm not even sure if it would work on Windows (crlf)

@mbrodala
Copy link
Contributor Author

mbrodala commented Sep 7, 2018

Actually it works for many lines.

@helhum
Copy link
Member

helhum commented Sep 7, 2018

Well, ok it is the other way around. Only one output line is treated as valid, leading to a wrong result if the serialized string contains newlines. See the result of this code with an actual value produced by a subprocess.

We would need to wrap the actual output between magic strings and then cut out that value from in between.

And still, if this magical string would appear within the actual output it would break.

@mbrodala
Copy link
Contributor Author

mbrodala commented Sep 7, 2018

Guess we'd actually need a way to stop PHP from emitting these messages in subprocesses ...

helhum added a commit to helhum/TYPO3-Console that referenced this issue Sep 8, 2018
In order to not interfere with the actual output
of sub-processes, we suppress the native php error output
for starting a sub-process.

Any important error will be turned into an exception anyway,
which will result in the sub-process to fail.

Fixes: TYPO3-Console#751
helhum added a commit to helhum/TYPO3-Console that referenced this issue Sep 8, 2018
In order to not interfere with the actual output
of sub-processes, we suppress the native php error output
for starting a sub-process.

Any important error will be turned into an exception anyway,
which will result in the sub-process to fail.

Also fix the non working PHP_INI_PATH env var setting
since we are at it.

Fixes: TYPO3-Console#751
@helhum
Copy link
Member

helhum commented Sep 8, 2018

This means if this option is used, even if set to Off the deprecation message is shown

I can't confirm, that the message appears when the option is set to off. Can you re-check? Thanks.

It does not appear, if track_errors is On, but display_errors is Off
It does not appear, if track_errors is Off, but display_errors is On

Therefore, I think, I have found a good solution to fix this.
Can you check whether #752 works for you?

@mbrodala mbrodala changed the title Suprocess failure on "upgrade:all" with PHP deprecation messages Subprocess failure on "upgrade:all" with PHP deprecation messages Sep 10, 2018
helhum added a commit to helhum/TYPO3-Console that referenced this issue Sep 29, 2018
In order to not interfere with the actual output
of sub-processes, we suppress the native php error output
for starting a sub-process.

Any important error will be turned into an exception anyway,
which will result in the sub-process to fail.

Also fix the non working PHP_INI_PATH env var setting
since we are at it.

Fixes: TYPO3-Console#751
helhum added a commit to helhum/TYPO3-Console that referenced this issue Oct 2, 2018
In order to not interfere with the actual output
of sub-processes, we suppress the native php error output
for starting a sub-process.

Any important error will be turned into an exception anyway,
which will result in the sub-process to fail.

Also fix the non working PHP_INI_PATH env var setting
since we are at it.

Fixes: TYPO3-Console#751
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants