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

Introduce signaled process specific exception class #25775

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
7 participants
@Soullivaneuh
Contributor

Soullivaneuh commented Jan 12, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25768
License MIT
Doc PR N/A

Introduced the ProcessSignaledException class to properly catch signaled process errors.

I took benefit to refactor process exception with a new ProcessRuntimeException class.

@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Jan 12, 2018

The failing job does not seem to be related to my PR: https://travis-ci.org/symfony/symfony/jobs/328083230#L3297

class ProcessRuntimeException extends RuntimeException
{
/**
* @var Process

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 12, 2018

Member

Do we need these PHPdoc now that we use type hints and return types?

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Jan 12, 2018

Contributor

Probably not, indeed.

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Jan 12, 2018

Contributor

Well, I maybe spoke to fast. The PHPdoc is for the property, not the constructor.

It could be remove from getProcess indeed, but I don't know for this one.

This comment has been minimized.

@iltar

iltar Jan 12, 2018

Contributor

You can remove it, the constructor already determines the type of this property. Your IDE will be able to detect this.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Jan 16, 2018

/**
* @author Sullivan Senechal <soullivaneuh@gmail.com>
*/
class ProcessRuntimeException extends RuntimeException

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 16, 2018

Member

does this intermediate class make sense in terms of exception hierarchy?
I'm not sure it does - let's use a trait instead? (it should be @internal)

This comment has been minimized.

@stof

stof Jan 16, 2018

Member

or even nothing at all, as the only shared code is the getProcess method, which has no business logic. Importing the trait would be almost as big as writing the getter.

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Jan 17, 2018

Contributor

I did this to avoid code duplicate. But right, I'll revert this part.

@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Jan 17, 2018

@nicolas-grekas @stof I did the changes you requested.

{
$this->process = $process;
parent::__construct(sprintf(

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 17, 2018

Member

Can you please put this call on one line? That's our CS :)

@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Jan 22, 2018

parent::__construct(sprintf('The process has been signaled with signal "%s".', $process->getTermSignal()));
}
public function getProcess()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 22, 2018

Member

missing return type, isn't it?

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Jan 22, 2018

Contributor

Well, I did a copy/paste from ProcessFailedException for that.

Plus, see this comment: #25775 (review)

I'm not sure I have to add it.

This comment has been minimized.

@Soullivaneuh

Soullivaneuh Jan 22, 2018

Contributor

Or maybe are you talking about PHP7 return type?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 22, 2018

Member

yes, I mean public function getProcess(): Process, sorry for the confusion

@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Jan 22, 2018

@nicolas-grekas Code updated.

@fabpot

fabpot approved these changes Jan 23, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 23, 2018

Thank you @Soullivaneuh.

@fabpot fabpot merged commit 68adb3b into symfony:master Jan 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 23, 2018

feature #25775 Introduce signaled process specific exception class (S…
…oullivaneuh)

This PR was merged into the 4.1-dev branch.

Discussion
----------

Introduce signaled process specific exception class

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25768
| License       | MIT
| Doc PR        | N/A

Introduced the `ProcessSignaledException` class to properly catch signaled process errors.

I took benefit to refactor process exception with a new `ProcessRuntimeException` class.

Commits
-------

68adb3b Introduce signaled process specific exception class

@Soullivaneuh Soullivaneuh deleted the Soullivaneuh:signaled-process-exception branch Jan 23, 2018

@Soullivaneuh

This comment has been minimized.

Contributor

Soullivaneuh commented Jan 23, 2018

You are very welcomed @fabpot! 😉

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment