Introduce signaled process specific exception class #25775
Conversation
|
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 |
There was a problem hiding this comment.
Do we need these PHPdoc now that we use type hints and return types?
There was a problem hiding this comment.
Probably not, indeed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You can remove it, the constructor already determines the type of this property. Your IDE will be able to detect this.
5ff9574 to
21f8ac0
Compare
| /** | ||
| * @author Sullivan Senechal <soullivaneuh@gmail.com> | ||
| */ | ||
| class ProcessRuntimeException extends RuntimeException |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I did this to avoid code duplicate. But right, I'll revert this part.
2b2cc07 to
b899f51
Compare
|
@nicolas-grekas @stof I did the changes you requested. |
| { | ||
| $this->process = $process; | ||
|
|
||
| parent::__construct(sprintf( |
There was a problem hiding this comment.
Can you please put this call on one line? That's our CS :)
b899f51 to
a8681e9
Compare
|
@nicolas-grekas done. |
| parent::__construct(sprintf('The process has been signaled with signal "%s".', $process->getTermSignal())); | ||
| } | ||
|
|
||
| public function getProcess() |
There was a problem hiding this comment.
missing return type, isn't it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Or maybe are you talking about PHP7 return type?
There was a problem hiding this comment.
yes, I mean public function getProcess(): Process, sorry for the confusion
|
@nicolas-grekas Code updated. |
a8681e9 to
18e7f43
Compare
18e7f43 to
68adb3b
Compare
|
Thank you @soullivaneuh. |
…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
|
You are very welcomed @fabpot! 😉 |
Introduced the
ProcessSignaledExceptionclass to properly catch signaled process errors.I took benefit to refactor process exception with a new
ProcessRuntimeExceptionclass.