Repository navigation
Fix PHP Monolog LogsHandler constructor in logs docs - #19640
Hashim1999164 wants to merge 2 commits into
Conversation
LogsHandler in sentry/sentry 4.x only takes a level (and bubble). The logs setup snippet still passed a Hub, so the sample could not run.
|
@Hashim1999164 is attempting to deploy a commit to the Sentry Team on Vercel. A member of the Team first needs to authorize it. |
| ]); | ||
|
|
||
| $log = new Logger('app'); | ||
| $log->pushHandler(new \Sentry\Monolog\LogsHandler( |
There was a problem hiding this comment.
https://github.com/getsentry/sentry-php/blob/75d09d8df75298c165961697be93a88e7dfbde78/src/Monolog/LogsHandler.php#L38 this misses the second argument.
There was a problem hiding this comment.
good catch, LogsHandler is (logLevel, bubble). updated the sample to pass true for bubble. Monolog\Level::Info is still valid there too (constructor takes LogLevel|Monolog\Level|int).
LogsHandler takes (logLevel, bubble). Keep the level and set bubble true so the sample matches the constructor.
0a08b13 to
da3cf18
Compare
|
good catch, LogsHandler is (logLevel, bubble). updated the sample to pass true for bubble. |
| hub: \Sentry\SentrySdk::getCurrentHub(), | ||
| level: Level::Info, | ||
| )); | ||
| $log->pushHandler(new \Sentry\Monolog\LogsHandler(Level::Info, true)); |
There was a problem hiding this comment.
Bug: The documentation example uses Monolog\Level::Info for the \Sentry\Monolog\LogsHandler constructor, but it likely expects a Sentry\Logs\LogLevel instance.
Severity: MEDIUM
Suggested Fix
In the code example, replace the usage of Monolog\Level::Info with Sentry\Logs\LogLevel::info(). Also, ensure the corresponding use statement is updated from use Monolog\Level; to use Sentry\Logs\LogLevel; to match the correct class.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: platform-includes/logs/setup/php.mdx#L25
Potential issue: The documentation example in `platform-includes/logs/setup/php.mdx`
instantiates `\Sentry\Monolog\LogsHandler` using `Monolog\Level::Info`, which is a
`Monolog\Level` enum case. However, other comprehensive documentation for this
integration uses `Sentry\Logs\LogLevel::info()`, which is a method call returning a
`Sentry\Logs\LogLevel` object. If the `LogsHandler` constructor is type-hinted for
`Sentry\Logs\LogLevel`, passing the `Monolog\Level` enum will cause a `PHP TypeError` at
runtime, making the example code non-functional.
Did we get this right? 👍 / 👎 to inform future reviews.
DESCRIBE YOUR PR
Fixes #19177
The PHP logs setup snippet still constructed LogsHandler with a Hub argument. In sentry/sentry 4.x that constructor only takes a log level (and bubble), so the sample failed as written. Updated it to match the working 4.x call.
IS YOUR CHANGE URGENT?
PRE-MERGE CHECKLIST
LEGAL BOILERPLATE
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Test plan