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.

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

Modernize the error handler #32565

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Modernize the error handler #32565

merged 2 commits into from
Nov 3, 2022

Conversation

ChristophWurst
Copy link
Member

  • Make it a dynamic class with dynamic methods
  • Make the logger a constructor arg -> it is always available
  • Fix that some deprecations were logged as errors (E_DEPRECATED vs E_USER_DEPRECATED)

@ChristophWurst ChristophWurst added this to the Nextcloud 25 milestone May 24, 2022
@ChristophWurst ChristophWurst self-assigned this May 24, 2022
@ChristophWurst
Copy link
Member Author

/backport 96a91cb to stable24

CarlSchwan
CarlSchwan reviewed May 24, 2022
View reviewed changes
lib/base.php Outdated Show resolved Hide resolved
lib/private/Log/ErrorHandler.php Outdated Show resolved Hide resolved
lib/private/Log/ErrorHandler.php Outdated Show resolved Hide resolved
CarlSchwan
CarlSchwan previously approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Looks good, just need a cs:fix run

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 24, 2022
@ChristophWurst
Copy link
Member Author

Tests fail

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jun 1, 2022
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 9, 2022
@ChristophWurst
Copy link
Member Author

I've moved the handler registration outside the constructor to make the class testable.

This was referenced Aug 12, 2022
Merged
Merged
@blizzz blizzz mentioned this pull request Aug 24, 2022
Merged
@blizzz blizzz mentioned this pull request Aug 30, 2022
Merged
come-nc
come-nc reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

👍

lib/base.php Outdated Show resolved Hide resolved
This was referenced Sep 6, 2022
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
This was referenced Sep 20, 2022
Closed
Merged
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@ChristophWurst ChristophWurst requested review from chiris90 and come-nc and removed request for chiris90 October 31, 2022 13:53
@ChristophWurst
Refactor the ErrorHandler into a dynamic class
052dcde 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Log all deprecations with debug level
4c8ec6d 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@come-nc
Copy link
Contributor

come-nc commented Nov 3, 2022

@ChristophWurst Will tests still fail on deprecation warnings? We have convertDeprecationsToExceptions="true" in the phpunit configuration and it’s quite useful to ensure PHP compatibility.

@ChristophWurst
Copy link
Member Author

@ChristophWurst Will tests still fail on deprecation warnings? We have convertDeprecationsToExceptions="true" in the phpunit configuration and it’s quite useful to ensure PHP compatibility.

Looks like they do

This attribute configures whether E_DEPRECATED and E_USER_DEPRECATED events triggered by the code under test are converted to an exception (and mark the test as error).

@ChristophWurst ChristophWurst merged commit ac92d00 into master Nov 3, 2022
@ChristophWurst ChristophWurst deleted the chore/modernize-error-handler branch November 3, 2022 13:42
@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@ChristophWurst
Copy link
Member Author

/backport 4c8ec6d to stable25

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Nov 3, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chiris90 chiris90 chiris90 left review comments

@come-nc come-nc come-nc approved these changes

@st3iny st3iny st3iny approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@J0WI J0WI Awaiting requested review from J0WI

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review Waiting for reviews technical debt
Projects
💌 📅 👥 Groupware team
Archived in project
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
7 participants
@ChristophWurst @come-nc @st3iny @chiris90 @CarlSchwan @blizzz @skjnldsv