PHP 8.2 Migration Prep: Logging Changes to support latest version of Monolog

[ryanrath]

Motivation and Context

The latest version of Monolog utilizes more of PHP's type system which is awesome in general, but causes us some issues since we log arrays instead of strings in many places. Thankfully, Monolog made the argument type string|\Stringable and not just string so there's a way forward that enables us to maintain the logging output we expect to the console, files, and the database while also allowing us to be compliant with the new version of Monolog and keeping the required changes to our code base as minimal as possible. Oh, and all of this is backwards compatible with our current version of Monolog / PHP, hence why this PR can go in prior to the Symfony changes.

We're going ahead with making all of our logging calls PSR3 compliant, i.e. no more directly logging arrays as messages. Instead if the previously logged array has a message property, that is now supplied as the $message argument, and the rest of the array is provided as the $context.

To accommodate these changes CCRLineFormatter was updated to to ensure that the $format will always have a %context% variable which will in turn be replaced with the same stringified output that was previously used for the array messages, thus maintaining our required formatting for console and file output.

Description of Changes

Here are the functional code changes I made, after this block I've listed all the other files that were only changed to either Wrap the logging of an array with LogOutput update the log call w/ the argument arrangement (as described above), or update the old short version of the log function calls ( err / crit ) to the current standard long versions ( error / critical ).

• classes/CCR/CCRDBHandler:
  • I updated the property that is being written to the message column to be formatted as opposed to message. This is so that the we can be sure that we're writing the json_encode'd message that was populated by the new processor.
• classes/CCR/Log:
  • The functional change made to this file is the update I made to the getDbHandler function.
    • I added a new LineFormatter with a format of %formatted% to the handler so that the value that will be associated the $record['formatted'].
    • I added a processor ( anonymous function ) that will take the value stored in the log record['extra']['message'] and store the json_encode'd value to record['formatted'], which will ultimately used by the CCRDBHandler & LineFormatter.
  • The non-functional changes I made:
    • Switching from the $value = isset($conf['value']) ? $conf['value] : $defaultValue to the much more compact $value = $conf['value'] ?? $defaultValue.
    • Updating some of the documentation spacing.
• classes/CCR/CCRLineFormatter:
  • After going through the Monolog LineFormatter architecture it turns out that we really only need to overwrite one function instead of two. I chose to make these changes in part because in the updated version of Monolog they've moved to using a class LogRecord instead of just using arrays, in addition they've made it so that you can only update particular properties of said record. This is why the CCR\Logger class adds the raw $message to the extra section as it's only one of a couple properties we can update.
  • I updated the constructor so that it ensures we always have a %context% variable that we can replace w/ the array provided as the $context argument.
  • I also found that there is a base function toJson that was already present / used to format arrays. By overriding this function I was able to achieve the log output we expect for console logging and it ends up being less code.
• classes/CCR/LogOutput:
  • This is the only completely new class and it's there to serve as a wrapper around array's of data passed to loggers. In addition to storing the original array for later use by our Processor, it also implements \Stringable so if you cast the object to a string you'll end up with what we'd expect for console or file logging.
  • This class is no longer needed and has been removed.
• tests/integration/lib/Logging/CCRDBHandlerTest:
  • I removed the manual addition of the CCRDBHandler as it's already present when we get a Logger w/ a DB Handler.
  • I also manually set the dbLogLevel to DEBUG so that we would see output from the $logger->debug call.

Important changes but does not affect the way the code ultimately functions:

• tests/playwright/Docker/docker-compose.yml:
  • Wrapped the port mapping in double quotes.
  • Changed the external port from 9006 to 8080 as a more standard port.

Tests performed

All Automated Tests

Checklist:

• The pull request description is suitable for a Changelog entry
• The milestone is set correctly on the pull request
• The appropriate labels have been added to the pull request

[jpwhite4]

This seems like a lot of code changes. What was the reason for not putting the adapter code in the log class instead of changing all the files that call the log functions?

[jpwhite4]

Just need explaination of why the code changes were not put in our existing custom shim class that already acts as the layer between out code and monolog code..

[ryanrath]

@jpwhite4 lol, I wrote out all the changes except for that one facepalm It definitely is, the reason I didn't put the update in the shim class is because when we finally upgrade to the up to date monolog version w/ Symfony + PHP 8.2 == the function signature for the log methods of Psr/Log/LoggerInterface has changed to string|\Stringable ( and we end up not needing a custom shim class ). So any class we create that implements LoggerInterface would need to follow these signatures. I guess we could write a custom log class that doesn't implement LoggerInterface but utilizes an instance of Monolog\Logger to do the actual logging which would allow us to have the same functions but with a different signature. But this seemed super cluudgy, when we should probably just be logging strings in the first place. I'm totally up for more discussion tomorrow about it.

[jpwhite4]

Is this change intended to be in this pull request?