Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 18, 2019
Merged

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

merged 1 commit into from
Jul 18, 2019

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jul 10, 2019 •
edited
Loading

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 #32470

@yceruto yceruto requested a review from sroze as a code owner July 10, 2019 02:55
@yceruto yceruto added this to the next milestone Jul 10, 2019
@yceruto
Copy link
Member Author

yceruto commented Jul 10, 2019

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

@yceruto yceruto force-pushed the error_handler_component branch from bdc7b48 to 1b48738 Compare July 10, 2019 03:52
@yceruto yceruto force-pushed the error_handler_component branch 2 times, most recently from 009cd5a to b842ad9 Compare July 10, 2019 14:12
@nicolas-grekas
Copy link
Member

rebase needed

@nicolas-grekas
Copy link
Member

Don'tmiss updating the root composer.json file.

@yceruto yceruto force-pushed the error_handler_component branch from b842ad9 to ca9ad2b Compare July 11, 2019 12:31
@yceruto
Copy link
Member Author

yceruto commented Jul 11, 2019

Done!

nicolas-grekas
nicolas-grekas reviewed Jul 11, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-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?

src/Symfony/Component/ErrorHandler/composer.json
"require": {
"php": "^7.1.3",
"psr/log": "~1.0",
"symfony/error-renderer": "^4.4|^5.0"
Copy link
Member

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
Copy link
Member Author

yceruto commented Jul 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

@yceruto yceruto force-pushed the error_handler_component branch from f29cec4 to 3d94a91 Compare July 11, 2019 14:28
@yceruto yceruto force-pushed the error_handler_component branch from 3d94a91 to 8201718 Compare July 11, 2019 15:16
@yceruto
Copy link
Member Author

yceruto commented Jul 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
Member Author

yceruto commented Jul 11, 2019

Maybe you have another solution?

@yceruto yceruto mentioned this pull request Jul 12, 2019
Closed
@yceruto yceruto force-pushed the error_handler_component branch from 8201718 to e9b10af Compare July 12, 2019 23:15
@yceruto
Copy link
Member Author

yceruto commented Jul 12, 2019

@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

Tobion commented Jul 13, 2019

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

Tobion
Tobion reviewed Jul 13, 2019
View reviewed changes
src/Symfony/Component/ErrorHandler/ExceptionHandler.php
*
* @return static
*/
public static function register($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
Member Author

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

Tobion commented Jul 15, 2019

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.

@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.

@nicolas-grekas nicolas-grekas mentioned this pull request Jul 17, 2019
Closed
@fabpot fabpot force-pushed the error_handler_component branch from 0c5070f to b1b6e80 Compare July 18, 2019 07:54
@fabpot
Copy link
Member

fabpot commented Jul 18, 2019

Thank you @yceruto.

@fabpot fabpot mentioned this pull request Jul 18, 2019
Closed
@fabpot fabpot merged commit b1b6e80 into symfony:4.4 Jul 18, 2019
fabpot added a commit that referenced this pull request Jul 18, 2019
@fabpot
feature #32471 Add a new ErrorHandler component (mirror of the Debug …
9ed1dd1 
…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 #32470

Commits
-------

b1b6e80 Add a new ErrorHandler component (mirror of the Debug component)
@yceruto yceruto deleted the error_handler_component branch July 18, 2019 13:03
@yceruto yceruto mentioned this pull request Jul 18, 2019
Merged
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tobion Tobion Tobion left review comments

@seriquynh seriquynh seriquynh left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Deprecation Feature Status: Reviewed
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