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

Add a new ErrorHandler component (mirror of the Debug component)#32471

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
fabpot merged 1 commit intosymfony:4.4fromyceruto:error_handler_component
Jul 18, 2019

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedJul 10, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

On top of#32470

@yceruto
Copy link
MemberAuthor

The PhpUnit bridge was affected here, hence the failures in Debug & ErrorHandler. This is solved automatically after the merge.

@ycerutoycerutoforce-pushed theerror_handler_component branch frombdc7b48 to1b48738CompareJuly 10, 2019 03:52
@ycerutoycerutoforce-pushed theerror_handler_component branch 2 times, most recently from009cd5a tob842ad9CompareJuly 10, 2019 14:12
@nicolas-grekas
Copy link
Member

rebase needed

@nicolas-grekas
Copy link
Member

Don'tmiss updating the root composer.json file.

@ycerutoycerutoforce-pushed theerror_handler_component branch fromb842ad9 toca9ad2bCompareJuly 11, 2019 12:31
@yceruto
Copy link
MemberAuthor

Done!

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

What about failures?

"require": {
"php":"^7.1.3",
"psr/log":"~1.0",
"symfony/error-renderer":"^4.4|^5.0"

Choose a reason for hiding this comment

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

something we might want to relax later on

yceruto reacted with thumbs up emoji
@yceruto
Copy link
MemberAuthor

yceruto commentedJul 11, 2019•
edited
Loading

What about failures?

#32471 (comment)
The PhpUnit bridge was affected here, hence the failures in Debug & ErrorHandler. This is solved automatically after the merge.

Let's see after#32471 (comment) what happen

@ycerutoycerutoforce-pushed theerror_handler_component branch from3d94a91 to8201718CompareJuly 11, 2019 15:16
@yceruto
Copy link
MemberAuthor

yceruto commentedJul 11, 2019•
edited
Loading

The problem with failed tests is related to these changes, because phpunit is still installing the old version (with Debug\DebugClassLoader).

But I fixed it to make it pass after the merge, when both are together, where the ErrorHandler\DebugClassLoader wins.

@yceruto
Copy link
MemberAuthor

Maybe you have another solution?

@yceruto
Copy link
MemberAuthor

@nicolas-grekas I fixed the failed tests, AppVeyor's failures are unrelated, I guess, and Travis's (high) is normal.

Status: Needs Review

@Tobion
Copy link
Contributor

What's the goal of this? It's just a renaming?

*
* @return static
*/
publicstaticfunctionregister($debug =true,$charset =null,$fileLinkFormat =null)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing types in all of the new ErrorHandler component

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I gonna make those changes in another PR with that goal, where we can see the diff clearer, now that would complicate the review unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with yceruto. Adding the ErrorHandler component is a big PR. So we should take care type-hints in an another one.

@Tobion
Copy link
Contributor

What do we gain from this apart from the slightly better fitting name? This will affect everyone and there is no automatic update because the Debug class is part of every front controller which is part of flex. I think some more justification would be good.

Koc reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

I see two major gains:

  • the name: "Debug" is a bad name for a piece of code that should really be loaded 100% of the time. This makes it hard to discover, understand, etc. Naming things correctly is critical.
  • big cleanup possible: not yet done in this PR, but Debug has some heavy logic from pre-PHP7 era, that a new component will allow cleaning up. Doing so in a smoother way would be too hard for the resources we have I believe.

@fabpotfabpotforce-pushed theerror_handler_component branch from0c5070f tob1b6e80CompareJuly 18, 2019 07:54
@fabpot
Copy link
Member

Thank you@yceruto.

@fabpotfabpot merged commitb1b6e80 intosymfony:4.4Jul 18, 2019
fabpot added a commit that referenced this pull requestJul 18, 2019
…component) (yceruto)This PR was squashed before being merged into the 4.4 branch (closes#32471).Discussion----------Add a new ErrorHandler component (mirror of the Debug component)| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -On top of#32470Commits-------b1b6e80 Add a new ErrorHandler component (mirror of the Debug component)
@ycerutoyceruto deleted the error_handler_component branchJuly 18, 2019 13:03
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@srozesrozeAwaiting requested review from sroze

+2 more reviewers

@TobionTobionTobion left review comments

@seriquynhseriquynhseriquynh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

6 participants

@yceruto@nicolas-grekas@Tobion@fabpot@seriquynh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp