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

Prevent PropertBag from being so noisy about deprecation warnings #16390

Merged
merged 1 commit into from
Jan 28, 2020

Conversation

artfulrobot
Copy link
Contributor

Overview

Turns out calling Civi::log() can be problematically slow (like taking over 0.5s/call) on some hosting environments. Currently there's a lot of core that is not updated to use Civi\Payment\PropertyBag yet, and that class was deliberately built to make noise about this. This resulted in a problematic performance hit for sites with slow logging.

See https://chat.civicrm.org/civicrm/pl/c9bwzmtrg7dhircfyfei5fezge

Before

PropertyBag would emit Civi::warning() synchronously every time it encountered deprecated use.

After

PropertyBag now stores all these complaints up and registers a shutdown function to write them to the log in one go.

@civibot
Copy link

civibot bot commented Jan 27, 2020

(Standard links)

@civibot civibot bot added the master label Jan 27, 2020
@artfulrobot
Copy link
Contributor Author

Nb. It also tags that message with civi.tag => deprecated - I'm not sure if that's desired or not.

@artfulrobot artfulrobot force-pushed the tame-propbag-deprecation-logs branch from afd4538 to afbfe83 Compare January 27, 2020 12:06
@artfulrobot
Copy link
Contributor Author

Tagging @mattwire and @totten who both expressed opinions on this in chat.

@mattwire
Copy link
Contributor

Thanks @artfulrobot - tested and confirmed this fixes the issue.

@mattwire mattwire merged commit 7c786c4 into civicrm:master Jan 28, 2020
@artfulrobot
Copy link
Contributor Author

awesome. good reminder that we need to go back through the code that's calling it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants