ENH: Use the PsrLogMessageProcessor so that custom factory created loggers can use context.

[LiamKearn]

Primarily I'm keen to avoid 'sprintf'ing into monolog as it's a bit of an anti-pattern and ruins the the use of the custom factory to some degree since monolog components can't make use of the context.

I think PsrLogMessageProcessor is a perfect change as it should keep the logging format the exact same (coverable by tests, if they exist) and keep the diff down.

Can we use enums in SS code style? I saw the module was ^8.1 but feel a bit unsure about that.

[LiamKearn]

Attaching some slack context:

Michal Kleiner (https://silverstripe-users.slack.com/archives/CCP2NDQTX/p1685967706352619?thread_ts=1685493270.463219&cid=CCP2NDQTX)

Had a quick look already. The changes make sense. I’m of two minds whether to always use explicit variable $context or not, but I would probably prefer it (since I’m raising it here). So even if there are no extra operations/business logic needed and the data can be passed straight to the logger call, I would probably always create a $context variable above the call and then pass that to the call.

[LiamKearn]

I get that, on one hand the point of context is to know what a scope looks like at the particular line that is being logged but on the other hand this isn't surrounded by application logic. It's intent is solely to log so having context inline is ugly when you want to optionally add to it.

I'll change the couple I inlined back and extract the rest of them as well on that note.

[LiamKearn]

Hey @michalkleiner,

I've:

• Extracted out the $context to as early in each method (usually before early returns) as possible to make conditional context look less ugly.
• Deleted the hacky custom SilverStripe\Auditor\Tests\Logger created to inspect a record for testing and replaced it with a normal monolog logger that uses the normal monolog pattern of a TestHandler. Also pushed a PsrLogProcessor onto that one so that there is little change needed to the actual test assertion content.
• Updated the tests to use monolog APIs to access context or the last logged message (this isn't entirely DRY but I wasn't sure of a good spot to put the repeated lastRecord and lastMessage methods, any ideas on this?

All tests are now passing with the singular exception of testOnBeforeRemoveLoginSession from session-manager which I'm not even sure is a regression here. I haven't looked very deeply at it but I'm pretty sure it's testing functionality that doesn't even exist anymore (admin can no longer delete other user's sessions, see below for details).

Noting that I haven't reviewed the diff yet because I'm still not sure how to move forward with that session-manager test, there might be quite a few silly bits'n bobs.

I'll revisit this in a few days to clean that up and work on any feedback.

---

So onwards to the rant which is maybe more of something for the CMS team ! ->

I think this has been mentioned before in PRs on this module but the way session-manager and mfa are included here is a bit of a pain. Here are a few things I think are already broken (not regressed within this PR):

• MFA/session-manager installations don't get verified by composer. There is virtually no API compatibility enforced between the modules if CI is not run (eg. most end users). I don't work with libraries so have little ideas here but I'd think it would be possible to say "Hey composer, if silverstripe/mfa is installed we require that it is xyz version constrained, but we do not require that it is installed"?
• For example here are some of the API issues I encountered with session-manager while testing:
  • Name spaces have changed from *\Model\* -> *\Models\*. There are inline type hints (exception causing ones) that use this so I'm surprised CI is working with silverstripe/session-manager @ ^2.
  • ENH Display toast notifications silverstripe-session-manager#39 has changed the removeLoginSession to just remove yet this module was still using removeLoginSession even whilst the CI points at: ^2.
  • The config for this module seems to register the SilverStripe\Auditor\AuditHookSessionManager as an extension on SilverStripe\SessionManager\Controllers\LoginSessionController instead of the LoginSession itself which now seems to be place for AuditHookSessionManager to listen for ::onBeforeRemoveLoginSession().
  • There is a test in this module that checks that an audit log is emitted when an admin removes a member's login session but as far as I can tell only the session's owner can delete their own session now?

I'm sort of thinking this more of something for the CMS team to look at, I think the audit hook module integrations belong in their respective modules not here. They use a singular API from silverstripe/auditor (the Injected AuditLogger service) whereas they use plenty of specific APIs from their own modules (silverstripe/mfa, silverstripe/session-manager).

CC @GuySartorelli any thoughts on how we minimised this API breakage from happening in the future?

[GuySartorelli]

I'd think it would be possible to say "Hey composer, if silverstripe/mfa is installed we require that it is xyz version constrained, but we do not require that it is installed"?

Probably conflicting with lower versions would be the way to do that.

For example here are some of the API issues I encountered with session-manager while testing

I only briefly glanced at this list but it sounds like some of these are genuine bugs - are there issues for them?

any thoughts on how we minimised this API breakage from happening in the future?

When did the API break? What specific breaks occurred? Other than the one you pointed to a specific commit to - this team was a bit looser about API breaks 2 years ago, so that specific example would be much less likely to happen today.

[LiamKearn]

I'd think it would be possible to say "Hey composer, if silverstripe/mfa is installed we require that it is xyz version constrained, but we do not require that it is installed"?

Probably conflicting with lower versions would be the way to do that.

For example here are some of the API issues I encountered with session-manager while testing

I only briefly glanced at this list but it sounds like some of these are genuine bugs - are there issues for them?

any thoughts on how we minimised this API breakage from happening in the future?

When did the API break? What specific breaks occurred? Other than the one you pointed to a specific commit to - this team was a bit looser about API breaks 2 years ago, so that specific example would be much less likely to happen today.

Probably conflicting with lower versions would be the way to do that.
I came across the same earlier but neglected to mention it because it's not as versatile as I'd like, it's a shame composer doesn't have a "require-version" section to compliment "require" and "require-dev".

Is conflicting something I can work on here or something that would be done across supported modules? If there are examples can you provide one so I can follow it's pattern for this.

I only briefly glanced at this list but it sounds like some of these are genuine bugs - are there issues for them?

Sounds like you want me to split the issues out and that is a good idea, I've started with #54 and will do a few more.

any thoughts on how we minimised this API breakage from happening in the future?

When did the API break? What specific breaks occurred? Other than the one you pointed to a specific commit to - this team was a bit looser about API breaks 2 years ago, so that specific example would be much less likely to happen today.

Probably should have been a bit more clear that I was referring to the removeLoginSession API change (public) that caused this module to break, issue for that here: #55