Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jul 18, 2019

Conversation

Projects
None yet
6 participants
@yceruto
Copy link
Member

commented Jul 10, 2019

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 Jul 10, 2019

@yceruto yceruto added this to the next milestone Jul 10, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

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 yceruto:error_handler_component branch from bdc7b48 to 1b48738 Jul 10, 2019

@yceruto yceruto added the Deprecation label Jul 10, 2019

@yceruto yceruto force-pushed the yceruto:error_handler_component branch 2 times, most recently from 009cd5a to b842ad9 Jul 10, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

rebase needed

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Don'tmiss updating the root composer.json file.

@yceruto yceruto force-pushed the yceruto:error_handler_component branch from b842ad9 to ca9ad2b Jul 11, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Done!

@nicolas-grekas
Copy link
Member

left a comment

What about failures?

Show resolved Hide resolved src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php Outdated
"require": {
"php": "^7.1.3",
"psr/log": "~1.0",
"symfony/error-renderer": "^4.4|^5.0"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 11, 2019

Member

something we might want to relax later on

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

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 yceruto:error_handler_component branch from f29cec4 to 3d94a91 Jul 11, 2019

@yceruto yceruto force-pushed the yceruto:error_handler_component branch from 3d94a91 to 8201718 Jul 11, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Maybe you have another solution?

@yceruto yceruto force-pushed the yceruto:error_handler_component branch from 8201718 to e9b10af Jul 12, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

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

*
* @return static
*/
public static function register($debug = true, $charset = null, $fileLinkFormat = null)

This comment has been minimized.

Copy link
@Tobion

Tobion Jul 13, 2019

Member

missing types in all of the new ErrorHandler component

This comment has been minimized.

Copy link
@yceruto

yceruto Jul 13, 2019

Author Member

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.

This comment has been minimized.

Copy link
@xuanquynh

xuanquynh Jul 13, 2019

Contributor

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

@Tobion

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

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

fabpot approved these changes Jul 18, 2019

@fabpot fabpot force-pushed the yceruto:error_handler_component branch from 0c5070f to b1b6e80 Jul 18, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Thank you @yceruto.

@fabpot fabpot merged commit b1b6e80 into symfony:4.4 Jul 18, 2019

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

fabpot added a commit that referenced this pull request Jul 18, 2019

feature #32471 Add a new ErrorHandler component (mirror of the Debug …
…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 yceruto:error_handler_component branch Jul 18, 2019

@yceruto yceruto referenced this pull request Jul 18, 2019

Open

Add ErrorHandler label #74

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.