Skip to content

Avoid Monolog\Logger interceptor generation and harden isHandling() against bootstrap errors - #252

Open
claudio-ferraro wants to merge 2 commits into
masterfrom
feature/magento-2.4.9-compatibility
Open

claudio-ferraro wants to merge 2 commits into
masterfrom
feature/magento-2.4.9-compatibility

Conversation

@claudio-ferraro

Copy link
Copy Markdown
Member

Summary

Investigating #242 (three reported circular DI fatals on Magento 2.4.9 setup:upgrade), I reproduced the crash on a fresh, isolated Magento 2.4.9 install (core modules + this module only), this directly answers @indykoning's question in the issue thread: it is not caused by an incompatibility with another extension, it reproduces standalone.

Isolating each of the three reported causes individually showed the actual FrontendPool ↔ CompiledConfig circular dependency is fully explained by cause 3, the Cache\Frontend\Factory constructor mismatch, that's the one genuinely Magento-2.4.9-specific bug here (2.4.9 added a new constructor argument to the core class), and it's already fixed on master (#239). With only that fix applied, and the Monolog\Logger plugin (cause 1) left completely untouched, setup:upgrade completes without error on a fresh 2.4.9 install.

So this PR is not a Magento 2.4.9 compatibility fix, causes 1 and 2 turn out not to be version-specific at all. The Monolog\Logger plugin has been unchanged since it was added in v4.3.0 and runs the same way on 2.4.8; Magento\Framework\Logger\Monolog and Magento's monolog/monolog constraint (^3.6) are identical between 2.4.8-p5 and 2.4.9. They only ever surfaced here because cause 3's TypeError happened to log through this exact path during bootstrap. Without a trigger like that, causes 1/2 sit dormant on any Magento version.

That said, they're real latent fragility worth hardening on their own merit:

  • di.xml registered a <plugin> directly on the third-party Monolog\Logger class. A plugin on a vendor class forces Magento to generate an Interceptor for it, which, depending on exactly when that first happens, can itself produce a circular dependency (FrontendPool → ResourceConnection → LoggerProxy → Monolog\Logger\Interceptor → PluginList::getNext() → CompiledConfig → FrontendPool, as also reported by @simonmaass). Wiring the handler into Magento's own Magento\Framework\Logger\Monolog via constructor argument (the same mechanism Magento itself uses for adding handlers) needs no interceptor at all, so Plugin/MonologPlugin.php is now unused and removed.
  • Logger\Handler\Sentry::isHandling() had no error handling. If anything gets logged while the DI container is still mid-construction, isHandling() resolves Helper\Data\Proxy → the real Helper\Data, whose constructor eagerly called collectModuleConfig() and could re-enter config/cache resolution that's already in progress. isHandling() now catches \Throwable and returns false, and Helper\Data no longer eagerly collects config in its constructor (it was already lazily memoized per store, so nothing changes for the normal path).

Related: #242

Result

Verified on Magento 2.4.9 (fresh install, core modules + JustBetter_Sentry only):

  • bin/magento setup:install / setup:upgrade / setup:di:compile all complete without errors, with the Sentry Monolog handler wired via constructor argument.
  • Regression-checked on an existing Magento 2.4.8-p5 project: setup:upgrade completes without any new errors.

Checklist

  • I've ran composer run codestyle
  • I've ran composer run analyse

Comment thread etc/di.xml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants