Updated headers for logger methods fixing compatibilty errors - #111
Conversation
23d2056 to
27177d0
Compare
27177d0 to
28ba7bd
Compare
There was a problem hiding this comment.
Pull request overview
Aligns the plugin’s logger method signatures with PSR-3’s PHP 8-compatible LoggerInterface method declarations to avoid signature compatibility fatals during PHPUnit runs.
Changes:
- Updated
Psr\Log\LoggerInterfacemethod signatures to usestring|\Stringableand: void. - Updated
Psr\Log\AbstractLoggermethod signatures to match the updated interface. - Updated
tool_allylogger base/DB logger method signatures (notablylog()to: void) and adjusted behavior on missing DB table.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| vendor/psr/log/Psr/Log/LoggerInterface.php | Changes the vendored PSR-3 interface method signatures to PHP 8-style typed signatures. |
| vendor/psr/log/Psr/Log/AbstractLogger.php | Updates convenience methods to match the new LoggerInterface signatures. |
| classes/logging/loggerdb.php | Updates log() signature and behavior; removes returning insert id. |
| classes/logging/loggerbase.php | Updates level-specific method signatures and the abstract log() signature to match PSR-3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function log($level, string|\Stringable $message, array $context = []): void { | ||
| global $DB; |
There was a problem hiding this comment.
loggerdb::log() now returns void, but tests/generator/lib.php currently relies on $logger->log() returning the inserted record id (and throws if it’s empty). This change will break the test generator (and any other callers expecting an id). Either update the generator/call sites to obtain the inserted id another way, or provide a separate API for inserting log records that returns the id while keeping the PSR-3 log() signature : void.
| if (!$DB->get_manager()->table_exists('tool_ally_log')) { | ||
| return; | ||
| throw new InvalidArgumentException(sprintf('The table tool_ally_log does not exist.')); | ||
| } |
There was a problem hiding this comment.
Throwing Psr\Log\InvalidArgumentException when the DB table is missing changes log() from a best-effort operation into a hard failure, and the exception type is misleading (missing table ≠ invalid argument/level). Since logging may be invoked before install/upgrade creates the table, prefer a no-op/early return (or a Moodle runtime exception with a clear operational message) rather than throwing an InvalidArgumentException here.
| * @param mixed $level | ||
| * @param string $message - this can be raw text, or if it starts with 'logger:' it will use an ally language string | ||
| * @param array $context | ||
| * @return null | ||
| */ | ||
| public function log($level, $message, array $context = []) { | ||
| public function log($level, string|\Stringable $message, array $context = []): void { |
There was a problem hiding this comment.
The docblock for log() is now out of sync with the method signature: it still documents $message as string and @return null, but the method accepts string|\Stringable and returns void. Please update the docblock to match the new signature (and consider documenting @throws behavior if exceptions are still possible).
Due to type strictness from PHP 8.x on some PHPUnit tests will throw a compatibility error like:
PHP Fatal error: Declaration of tool_ally\logging\loggerbase::emergency($message, array $context = []) must be compatible with Psr\Log\LoggerInterface::emergency(Stringable|string $message, array $context = []): void in /var/www/html/admin/tool/ally/classes/logging/loggerbase.php on line 54I have updated the method headers to follow the ones in LoggerInterface.