starred/monolog

Fork 0

mirror of https://github.com/Seldaek/monolog.git synced 2026-04-26 16:15:49 +03:00

[GH-ISSUE #1684] `addRecord` method signature change causes Fatal Error #719

New issue

Closed

opened 2026-03-04 02:17:22 +03:00 by kerem · 5 comments

kerem commented

2026-03-04 02:17:22 +03:00

Owner

Originally created by @convenient on GitHub (Jun 9, 2022).
Original GitHub issue: https://github.com/Seldaek/monolog/issues/1684

Raised in https://github.com/magento/magento2/pull/35596 but thought i'd raise here just in case its something you want to be aware of. Happy for this issue to be closed if it just something that needs to be handled in magento but think it may help people if they have similar frameworks / similar issues.

This version of monlog/monolog was tagged about an hour ago https://github.com/Seldaek/monolog/releases/tag/2.7.0

Which goes along with this commit
github.com/Seldaek/monolog@0ddba7342f (diff-1e7fd545ce)

This changes the method signature of addRecord like so

-    public function addRecord(int|Level $level, string $message, array $context = []): bool
+    public function addRecord(int|Level $level, string $message, array $context = [], DateTimeImmutable $datetime = null): bool

If you have PHP 8.1 class extending that class and method it will now get a PHP error like so (depending on what your class is called)

PHP Fatal error:  Declaration of Magento\TestFramework\ErrorLog\Logger::addRecord(int $level, string $message, array $context = []): bool must be compatible with Monolog\Logger::addRecord(int $level, string $message, array $context = [], ?Monolog\DateTimeImmutable $datetime = null): bool in dev/tests/integration/framework/Magento/TestFramework/ErrorLog/Logger.php on line 69

Originally created by @convenient on GitHub (Jun 9, 2022). Original GitHub issue: https://github.com/Seldaek/monolog/issues/1684 Raised in https://github.com/magento/magento2/pull/35596 but thought i'd raise here just in case its something you want to be aware of. Happy for this issue to be closed if it just something that needs to be handled in magento but think it may help people if they have similar frameworks / similar issues. --- This version of `monlog/monolog` was tagged about an hour ago https://github.com/Seldaek/monolog/releases/tag/2.7.0 Which goes along with this commit https://github.com/Seldaek/monolog/commit/0ddba7342ff919990e1914d24db15e565f153676#diff-1e7fd545cec457de96f5ed6bd7249ba091cd9e699b4057db15ff1e2e0364025bR297 This changes the method signature of `addRecord` like so ```diff - public function addRecord(int|Level $level, string $message, array $context = []): bool + public function addRecord(int|Level $level, string $message, array $context = [], DateTimeImmutable $datetime = null): bool ``` If you have PHP 8.1 class extending that class and method it will now get a PHP error like so (depending on what your class is called) ``` PHP Fatal error: Declaration of Magento\TestFramework\ErrorLog\Logger::addRecord(int $level, string $message, array $context = []): bool must be compatible with Monolog\Logger::addRecord(int $level, string $message, array $context = [], ?Monolog\DateTimeImmutable $datetime = null): bool in dev/tests/integration/framework/Magento/TestFramework/ErrorLog/Logger.php on line 69 ```

kerem

2026-03-04 02:17:22 +03:00

closed this issue
added the
Bug
label

kerem commented

2026-03-04 02:17:23 +03:00

Author

Owner

@gabesullice commented on GitHub (Jun 10, 2022):

Commenting to note that this change also caused downstream issues in the Drupal monolog module: https://www.drupal.org/project/monolog/issues/3284825

@gabesullice commented on GitHub (Jun 10, 2022): Commenting to note that this change also caused downstream issues in the Drupal monolog module: https://www.drupal.org/project/monolog/issues/3284825

kerem commented

2026-03-04 02:17:23 +03:00

Author

Owner

@Seldaek commented on GitHub (Jun 11, 2022):

Sorry for the problems here, I somehow didn't realize this was going to have an impact on downstream users.

That said, extending Logger is probably not a great idea, and extending addRecord an even worse one IMO, so I think I may just leave this as is.

The Drupal Monolog case is legit IMO and can probably be solved by https://github.com/Seldaek/monolog/issues/1686 - this would have been better as a feature request here than a hack.

The Magento usage seems to be only used for tests (is that correct?) so unlikely to affect all magento users? Your PR works @convenient I guess but really I do wonder why Logger was extended here instead of adding a custom Handler to collect less-than-error records. That's what handlers are for, overriding addRecord for this is madness IMO. You could even do it with native handlers from monolog combining a FilterHandler and a TestHandler (or just the latter..) to gather records and access them again in the test.

@Seldaek commented on GitHub (Jun 11, 2022): Sorry for the problems here, I somehow didn't realize this was going to have an impact on downstream users. That said, extending Logger is probably not a great idea, and extending addRecord an even worse one IMO, so I think I may just leave this as is. The Drupal Monolog case is legit IMO and can probably be solved by https://github.com/Seldaek/monolog/issues/1686 - this would have been better as a feature request here than a hack. The Magento usage seems to be only used for tests (is that correct?) so unlikely to affect all magento users? Your PR works @convenient I guess but really I do wonder why Logger was extended here instead of adding a custom Handler to collect less-than-error records. That's what handlers are for, overriding addRecord for this is madness IMO. You could even do it with native handlers from monolog combining a FilterHandler and a TestHandler (or just the latter..) to gather records and access them again in the test.

kerem commented

2026-03-04 02:17:23 +03:00

Author

Owner

@convenient commented on GitHub (Jun 12, 2022):

@Seldaek Agreed that this doesn't seem to be the correct way to do things in the Magento code, as I said I mainly flagged this in this repo so that it was at least recorded somewhere to be discussed 🙂

Apparently they also use that pattern in one of their optional modules https://github.com/magento/magento2/issues/35604

github.com/magento/data-migration-tool@4ef01e1073/src/Migration/Logger/Logger.php (L34-L39)

This would prevent Magento from being deployable if you use monolog 2.7.0 but i think that its up to magento to fix their code and update composer.json restraints to ensure its always valid.

@convenient commented on GitHub (Jun 12, 2022): @Seldaek Agreed that this doesn't seem to be the correct way to do things in the Magento code, as I said I mainly flagged this in this repo so that it was at least recorded somewhere to be discussed 🙂 Apparently they also use that pattern in one of their optional modules https://github.com/magento/magento2/issues/35604 https://github.com/magento/data-migration-tool/blob/4ef01e107379ad8bbe9ee6e7645b83718feab7ed/src/Migration/Logger/Logger.php#L34-L39 This would prevent Magento from being deployable if you use monolog 2.7.0 but i think that its up to magento to fix their code and update composer.json restraints to ensure its always valid.

kerem commented

2026-03-04 02:17:23 +03:00

Author

Owner

@dpi commented on GitHub (Jun 13, 2022):

That said, extending Logger is probably not a great idea, and extending addRecord an even worse one IMO, so I think I may just leave this as is.

Would you consider final -izing the method, or entire class? Perhaps as a v4 thing?

@dpi commented on GitHub (Jun 13, 2022): > That said, extending Logger is probably not a great idea, and extending addRecord an even worse one IMO, so I think I may just leave this as is. Would you consider `final` -izing the method, or entire class? Perhaps as a v4 thing?

kerem commented

2026-03-04 02:17:24 +03:00

Author

Owner

@Seldaek commented on GitHub (Jul 22, 2022):

@dpi I marked it with @final for now.

Closing this as it seems the problems were mostly mitigated upstream.

@Seldaek commented on GitHub (Jul 22, 2022): @dpi I marked it with `@final` for now. Closing this as it seems the problems were mostly mitigated upstream.

kerem referenced this issue

2026-03-04 03:09:22 +03:00

[PR #719] [CLOSED] Add Understand.io handler, formatter & tests #1313