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

Logging Refactor #2656

Closed
wants to merge 3 commits into from
Closed

Logging Refactor #2656

wants to merge 3 commits into from

Conversation

dafriend
Copy link
Contributor

@dafriend dafriend commented Mar 1, 2020

Description
Fix Chrome Logger per #2616
But wait! There's more...

  • It greatly enhances how the logger parses and displays PHP variables.
  • Prevents ChromeLogger from running in a production environment.
  • The documentation page was redone to accommodate the configuration changes and describe the logging of variables, etc.

Checklist:

  • [ x] Securely signed commits
  • [ x] Component(s) with PHPdocs
  • [ x] Unit testing, with >80% coverage
  • [ x] User guide updated
  • [?] Conforms to style guide

This might be a breaking change in that \Config\Logger has changed. Some new properties were added and others were effectively renamed.

However, the overall API is not changed and Logger still conforms to PSR-3.

MGatner and others added 3 commits March 1, 2020 12:07
Fixes for #2616
Also greatly enhances how the logger parses and dsiplays PHP data types.
Fixes for #2616
Also greatly enhances how the logger parses and dsiplays PHP data types.
Return it to the "as distributed" state.
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

I was not able to complete a review of this code. All of changes to the config file need to be rolled back. We are not anywhere close to being ready for a BC breaking change, and almost all of the changes were unnecessary, but do to a preference.

Once that has been refactored out I can review the logic changes without all of the changes that the config file changes required.

* should be written to the log file.
*
* Here are the values and the message type
* 0 - off
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the examples from this list?

*
* For a live site you'll usually want critical (3) or lower to be logged, otherwise
* your log files will fill up very fast.
*
Copy link
Member

Choose a reason for hiding this comment

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

This amount of text is more appropriate for user guide than a doc block. The list of levels was provided as a quick reference only.

*
* @var integer|array
*/
public $fileLevelsHandled = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Please change the name back as this is definitely a BC break since this requires a change in every application that has been started on the framework so far.

@dafriend dafriend closed this Mar 5, 2020
@dafriend dafriend deleted the Logging-Refactor branch March 5, 2020 17:44
@MGatner MGatner mentioned this pull request Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants