Doing a code review with Bruno, we agreed to do some little refactoring :
* Level per exception class
- Before the whole ExceptionLog::Log method was a total rewrite of its parent, with some code duplicates... not a good idea : we should better improve LogAPI to make other similar uses possible in the future !
- The logic to get level from config must be in a GetMinLogLevel override
* Write to DB
- Pull up this functionnality in LogAPI
- Add a sCode parameter in GetLevelDefault
Doing this refactoring, I also improved :
* Test the attributes set when creating the EventIssue object : during my dev I had crashes because I didn't filled all the mandatory fields... Having a PHPUnit test checking this will prevent future bugs to happen if attributes are modified in the class or in the object creation method
* Use Throwable instead of Exception : this was added in PHP 7.0 and will allow to catch both Exception and Error
* Because we need to have 2 statements on the same line in \Combodo\iTop\Test\UnitTest\Core\Log\ExceptionLogTest::testLogInFile, I modified the editorConfig file to allow disabling the formatter using comments.
plus:
- the entrypoint is now `LogException()` instead of `FromException()` which sounded more like a factory and less like an active method.
- merge conflicting commit with @molkobain (CC fix)
- remove the writing of the exception object in the error.log context (adding it was an error, it's way too verbose!!).
- Technical note: The context is still used to propagate the exception across several call stack, so it now uses a less generic naming in order to avoid conflicts (see `ExceptionLog::CONTEXT_EXCEPTION`). another solution would have been to add a new parameter to `ExceptionLog::Log()`, but I didn't want to add constraint over the hypothetical evolution of the base class method.
plus:
- the exception object is no more automatically added to the error.log context (it's way too verbose)
- code cleanup (use constant instead of repeated strings)
- ExceptionLog main method is now named `LogException` instead of `FromException`
Try to repair an odd error in the CI:
> Fatal error: Uncaught Exception: Serialization of 'ReflectionClass' is not allowed
avoid instantiation in the provider, delay them to the actual test
Such Exceptions are triggered with a Warning level and default the minimum default level in order to write in DB is above, so the behavior is not modified:
- logs are written in errors.log (with a warning elvel instead of an error)
- logs are not written in DB unless `log_level_min.write_in_db` is changed
They are logged as WARNING level in \DeprecatedCallsLog::ENUM_CHANNEL_PHP_LIBMETHOD channel
Such deprecated notices are generated inside Symfony for example using @trigger_error(..., E_DEPRECATED).
Having such a log will help us migrate to the next lib version !
Such notices are generated inside Symfony for example using @trigger_error(..., E_DEPRECATED).
Having such a log will help us migrate to the next version !
This was bringed by #193
The stack trace wasn't used correctly :/ This is now fixed !
Sample message for deprecated PHP method call :
`2021-04-13 15:31:47 | Warning | Call to WebPage:outputCollapsibleSectionInit in C:\...\itop-dev\sources\application\WebPage\AjaxPage.php#L170 (AjaxPage::output) | deprecated-php-method`
Log message for deprecated PHP file remains the same :
`2021-04-13 10:31:12 | Warning | C:\...\itop-dev\sources\application\WebPage\WebPage.php L32 including/requiring C:\...\itop-dev\core\coreexception.class.inc.php : Classes were moved to /application/exceptions | deprecated-file`
* New DeprecatedCallsLog logger enabled directly in bootstrap so that we can use it in the most situations
* Default Log level is now DEBUG for dev env, ERROR for others
* Logs the caller and the deprecated method called, like :
`2021-04-06 10:52:25 | Warning | AjaxPage::output L2857 calling WebPage:outputCollapsibleSectionInit | deprecated-method`
* Sample file consumer
* Sample php consumer
First log implementation (75730ee) was creating EventIssue objects, and was rollbacking transaction if it exists
The new one has some benefits :
* always log one line by default in log/error.log, but details must be activated though config (channels `Deadlock-WaitTimeout` and `Deadlock-Found`)
* detailed logs are in a dedicated file (log/deadlock.log) :
- easier for our clients to get and share
- has rotation by default
- looking at the file size is a direct way to know if error happened
- more compliant to industry standards !
* the transaction stays untouched, so that the consumer can do whatever it prefers
