Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Debug] Support any Throwable object in FlattenException#26528

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

Merged

Conversation

@derrabus
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#26429
LicenseMIT
Doc PRN/A

Alternative to#26447 without BC breaks, follows#26028.

This PR removes the need to convertThrowable objects into exceptions when working withFlattenException.

ping@instabledesign@ostrolucky@nicolas-grekas

@derrabusderrabus changed the titleSupport any Throwable object in FlattenException[Debug] Support any Throwable object in FlattenExceptionMar 14, 2018
@derrabusderrabusforce-pushed theflatten-exception-with-throwable branch from4813746 toba00693CompareMarch 14, 2018 14:15
*
* @return static
*/
publicstaticfunctioncreate($throwable,$statusCode =null,array$headers =array())
Copy link
Member

Choose a reason for hiding this comment

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

your#26447 (comment) comment applies as well, removing a typehint breaks childs

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

You're right, it will raise a warning. I'll add another deprecation then.

@derrabusderrabusforce-pushed theflatten-exception-with-throwable branch 4 times, most recently from10cef07 to9a2cf0cCompareMarch 14, 2018 15:49
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneMar 14, 2018
*/
publicstaticfunctioncreate(\Exception$exception,$statusCode =null,array$headers =array())
{
@trigger_error(sprintf('"%s" is deprecated since Symfony 4.1. Use "createFromThrowable()" instead.',__METHOD__),E_USER_DEPRECATED);

Choose a reason for hiding this comment

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

maybe we should not deprecate this method and save us the version bumps?

Copy link
Member

Choose a reason for hiding this comment

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

a lot of code in this class will never be reached in case the throwable is an\Error (seeinstanceof *ExtensionInterface checks).
If we really want a flatten error, couldn't it just be a newFlattenError? extracting the reusable logic in a trait if any

Copy link
Member

Choose a reason for hiding this comment

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

Tbh, given the error handling in place I fear that anyways the result will be more confusing than useful here, perhaps it's just me

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for FlattenError

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I did not expect those version bumps to be a problem, but we can add the depreciation at a later point, I guess. I'm going to remove the deprecation an revert the changes I made to other components.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

If I added a separate class for errors, I would need to duplicate a lot of code, lose the ability to properly type-hint for FlattenException and I'd still have to do type-switches when working with arbitrary throwables. Sorry, but this is really a bad idea, imho.

@derrabusderrabusforce-pushed theflatten-exception-with-throwable branch from9a2cf0c tof2782adCompareMarch 15, 2018 08:42
@derrabus
Copy link
MemberAuthor

I've remove the deprecation of thecreate method and rolled back the changes in TwigBundle and HttpKernel for now, as@nicolas-grekas suggested. PR is ready.

publicfunctiontestStatusCode()
{
$flattened = FlattenException::create(new \RuntimeException(),403);
$flattened = FlattenException::createFromThrowable(new \RuntimeException(),403);

Choose a reason for hiding this comment

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

unless required, these changes should also be reverted, this will help merges as usual

derrabus reacted with confused emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Understandable, although I'd prefer to keep the test as it is. I'll change it.

@derrabusderrabusforce-pushed theflatten-exception-with-throwable branch 3 times, most recently from4fdb120 to47b06a7CompareMarch 15, 2018 09:11
@derrabus
Copy link
MemberAuthor

All right, the change set should be minimal now. Still, my goal would be to remove thecreate method with Symfony 5, because it's redundant now.

publicstaticfunctioncreateFromThrowable(\Throwable$throwable, ?int$statusCode =null,array$headers =array()):self
{
$e =newstatic();
$e->setMessage($exception->getMessage());

Choose a reason for hiding this comment

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

sorry, anynother change: let's keep the old name, it's still fine

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fair enough.

@derrabus
Copy link
MemberAuthor

Status: Needs Review

Copy link
Contributor

@instabledesigninstabledesign left a comment

Choose a reason for hiding this comment

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

Sounds good.

@derrabusderrabusforce-pushed theflatten-exception-with-throwable branch fromf78a15a to25e5cdfCompareMarch 16, 2018 10:49
@derrabus
Copy link
MemberAuthor

The failure on Travis looks unrelated.

@derrabus
Copy link
MemberAuthor

Anything left to do here?

returnstatic::createFromThrowable($exception,$statusCode,$headers);
}

publicstaticfunctioncreateFromThrowable(\Throwable$exception, ?int$statusCode =null,array$headers =array()):self
Copy link
Member

Choose a reason for hiding this comment

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

What about usingnew instead (FlattenException::new() looks better to me)?

chalasr reacted with thumbs up emojiderrabus reacted with confused emoji
Copy link
Member

@nicolas-grekasnicolas-grekasApr 2, 2018•
edited
Loading

Choose a reason for hiding this comment

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

it's not possible to call a function "new" because the keyword is reserved...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Well, then the developer would have the choice betweencreate() andnew() which looks rather confusing to me. Also, I would not recommend to use keywords as method names even if it currently works.

publicfunctionsetTraceFromException(\Exception$exception)
{
$this->setTrace($exception->getTrace(),$exception->getFile(),$exception->getLine());
@trigger_error(sprintf('"%s" is deprecated since Symfony 4.1. Use "setTraceFromThrowable()" instead.',__METHOD__),E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

should be... since Symfony 4.1, use ...

derrabus reacted with thumbs up emoji
@fabpot
Copy link
Member

Thank you@derrabus.

@fabpotfabpot merged commit96c1a6f intosymfony:masterApr 6, 2018
fabpot added a commit that referenced this pull requestApr 6, 2018
…on (derrabus)This PR was merged into the 4.1-dev branch.Discussion----------[Debug] Support any Throwable object in FlattenException| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#26429| License       | MIT| Doc PR        | N/AAlternative to#26447 without BC breaks, follows#26028.This PR removes the need to convert `Throwable` objects into exceptions when working with `FlattenException`.ping@instabledesign@ostrolucky@nicolas-grekasCommits-------96c1a6f Support any Throwable object in FlattenException.
@derrabusderrabus deleted the flatten-exception-with-throwable branchApril 6, 2018 08:22
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestMay 2, 2018
…r (derrabus)This PR was merged into the master branch.Discussion----------FlattenException might also represent an instance of ErrorThis PR documentssymfony/symfony#26528 andfixes#9636.Commits-------cd309c9 FlattenException might also represent an instance of Error.
@fabpotfabpot mentioned this pull requestMay 7, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr left review comments

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@ostroluckyostroluckyostrolucky left review comments

@instabledesigninstabledesigninstabledesign approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

7 participants

@derrabus@fabpot@nicolas-grekas@ostrolucky@instabledesign@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp