Skip to content

[Process] Inherit the error_reporting from parent #12754

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

Closed
wants to merge 1 commit into from
Closed

[Process] Inherit the error_reporting from parent #12754

wants to merge 1 commit into from

Conversation

davidfuhr
Copy link

There are several builds failing due to the new E_DEPRECATED_USER messages. This is also an issue in the functional tests. The problem is that the PhpProcess started to run the functional test does not inherit the error_reporting setting from the parent.

Q A
Bug fix? yes
New feature? no
BC breaks? somewhat
Deprecations? yes
Tests pass? no (but broken upstream)
Fixed tickets
License MIT
Doc PR

Relates #12623
Relates #12705

@@ -53,6 +53,8 @@ public function __construct($script, $cwd = null, array $env = array(), $timeout
*/
public function setPhpBinary($php)
{
$php .= ' -d error_reporting=' . error_reporting();
Copy link
Member

Choose a reason for hiding this comment

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

You sholud remove the spaces around the dot.

@davidfuhr davidfuhr changed the title Inherit the error_reporting from parent [Process] Inherit the error_reporting from parent Nov 29, 2014
@romainneutron
Copy link
Contributor

Hello,

I don't think this the right fix. It looks like a special case here, and I personnaly don't like it. If we do that, we should inheriting all configuration set by Symfony, right? But I definitely don't think we should have this approach

@davidfuhr
Copy link
Author

Yes I agree. The child process should inherit all the configuration from the parent if not set otherwise. If this is the way to go I will look into that.

Or do you disagree fixing this in the Process component? And would prefer a fix in FrameworkBundle/Test?

@nicolas-grekas
Copy link
Member

The process component is fine, it should not inherit INI context. The correct fix to me is: update the process component or any other involved so that nothing deprecated is ever used, or, if the test is about testing some deprecated thing, add explicit INI settings in the test class

@romainneutron
Copy link
Contributor

@davidfuhr : I and @nicolas-grekas both agree that the Process Component should not be fixed. It should not inherit all the configuration from its parent.

I just said that if you want to update a configuration setting, you should do all. But after that I said I did not like this approach:

But I definitely don't think we should have this approach

@fabpot
Copy link
Member

fabpot commented Dec 1, 2014

Closing for the same reasons as the one given by @nicolas-grekas

@fabpot fabpot closed this Dec 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants