Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Introduce signaled process specific exception class#25775
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
soullivaneuh commentedJan 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need these PHPdoc now that we use type hints and return types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably not, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, I maybe spoke to fast. The PHPdoc is for the property, not the constructor.
It could be remove fromgetProcess indeed, but I don't know for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can remove it, the constructor already determines the type of this property. Your IDE will be able to detect this.
5ff9574 to21f8ac0Compare| /** | ||
| * @author Sullivan Senechal <soullivaneuh@gmail.com> | ||
| */ | ||
| class ProcessRuntimeException extends RuntimeException |
nicolas-grekasJan 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
or even nothing at all, as the only shared code is thegetProcess 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did this to avoid code duplicate. But right, I'll revert this part.
2b2cc07 tob899f51Comparesoullivaneuh commentedJan 17, 2018
@nicolas-grekas@stof I did the changes you requested. |
| { | ||
| $this->process = $process; | ||
| parent::__construct(sprintf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please put this call on one line? That's our CS :)
b899f51 toa8681e9Comparesoullivaneuh commentedJan 22, 2018
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing return type, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, I did a copy/paste fromProcessFailedException for that.
Plus, see this comment:#25775 (review)
I'm not sure I have to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or maybe are you talking about PHP7 return type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, I meanpublic function getProcess(): Process, sorry for the confusion
soullivaneuh commentedJan 22, 2018
@nicolas-grekas Code updated. |
a8681e9 to18e7f43Compare18e7f43 to68adb3bComparefabpot commentedJan 23, 2018
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/AIntroduced 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 commentedJan 23, 2018
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.