Skip to content

Conversation

@Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Dec 8, 2025

Description (*)

  • Fixes Mage::log when using PHP own Log_* constants.
  • Also adds Mage_Core_Helper_PsrLogger as way to implement LoggerInterface using Mage Log function

Related Pull Requests

Fixed Issues (if relevant)

  • fixes OpenMage/magento-lts#<issue_number>

Manual testing scenarios (*)

  1. use LOG_DEBUG on Mage::log
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@Hanmac Hanmac requested a review from sreichel December 8, 2025 10:42
@Hanmac Hanmac added the Component: Core Relates to Mage_Core label Dec 8, 2025
@github-actions github-actions bot added the Mage.php Relates to app/Mage.php label Dec 8, 2025
@sreichel sreichel added the bug label Dec 8, 2025
sreichel
sreichel previously approved these changes Dec 9, 2025
@addison74 addison74 marked this pull request as ready for review December 9, 2025 04:45
Copilot AI review requested due to automatic review settings December 9, 2025 04:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds PSR-3 LoggerInterface support to OpenMage by introducing Mage_Core_Helper_PsrLogger and extending Mage::log() to accept RFC 5424 numeric log levels (0-7) and PSR-3 string level names. This enables better integration with modern PHP logging libraries and tools while maintaining backward compatibility with existing code.

Key Changes

  • New Mage_Core_Helper_PsrLogger class provides PSR-3 LoggerInterface implementation wrapping Mage::log()
  • Enhanced Mage::log() to automatically convert RFC 5424 numeric levels (0-7, used by PHP's LOG_* constants) to corresponding Monolog levels
  • Added support for PSR-3 string level names ('debug', 'info', 'warning', etc.) via Level::fromName()

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
app/code/core/Mage/Core/Helper/PsrLogger.php New PSR-3 LoggerInterface implementation that wraps Mage::log(), enabling integration with PSR-3 compatible libraries
app/Mage.php Extended log level handling to support RFC 5424 numeric levels and PSR-3 string level names, with proper conversion to Monolog level values

Comment on lines 18 to 26
class Mage_Core_Helper_PsrLogger extends Mage_Core_Helper_Abstract implements LoggerInterface
{
use LoggerTrait;

public function log($level, string|\Stringable $message, array $context = []): void
{
Mage::log((string) $message, $level, null, false, $context);
}
}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The new Mage_Core_Helper_PsrLogger class lacks test coverage. Since other helpers in the same directory have unit tests (e.g., tests/unit/Mage/Core/Helper/DataTest.php), this class should also have a corresponding test file. Consider adding tests/unit/Mage/Core/Helper/PsrLoggerTest.php to verify:

  • The log() method correctly calls Mage::log() with the expected parameters
  • PSR-3 log levels (debug, info, warning, error, etc.) are properly passed through
  • The $context array is correctly forwarded
  • String and Stringable messages are handled correctly

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 9, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Hanmac
Copy link
Contributor Author

Hanmac commented Dec 9, 2025

Not an "accessibility bypass" like SonarQube thinks

@sreichel
Copy link
Contributor

Note: there is a PSR-Handler

There a serveral ideas to support mor handlers (and possible replace firegento/logger), Any help is welcome. :)

@Hanmac
Copy link
Contributor Author

Hanmac commented Dec 12, 2025

it is listed there, at the bottom of the handler in the "Wrappers / Special Handlers" section:

Can be used to forward log records to an existing PSR-3 logger

But this PR is a different direction:

  • the PsrHandler from Monolog is for "Monolog -> Psr"
  • this PR is for "Psr -> Magento -> Monolog"

@Hanmac
Copy link
Contributor Author

Hanmac commented Dec 12, 2025

(and possible replace firegento/logger)

That's what got me thinking first, i added firegento/logger to another project recently.
And after looking into how it works, my first thought was that it wasn't working with new Monolog changes anymore.

@sreichel
Copy link
Contributor

Only from reviewing the code ... i think changing log level to monolog values coud cause problems.

Updated PR to use RFC5424 levels.

@sreichel sreichel marked this pull request as draft December 14, 2025 20:51
@sreichel sreichel modified the milestones: 20.17.0, 20.18.0 Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Core Relates to Mage_Core enhancement hold Mage.php Relates to app/Mage.php

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants