addRecord method signature change causes Fatal Error

[convenient]

Raised in magento/magento2#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
0ddba73#diff-1e7fd545cec457de96f5ed6bd7249ba091cd9e699b4057db15ff1e2e0364025bR297

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

[gabesullice]

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

[Seldaek]

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 #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.

[convenient]

@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 magento/magento2#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.

[dpi]

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?

[Seldaek]

@dpi I marked it with @final for now.

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