refactor: offload plugin discovery to PluginDiscovery class#45
Conversation
- PluginDiscovery::discoverInModule() now returns PluginDefinition[] instead of raw file paths, eliminating duplicate extract/load/reflect work in Application - Requires ClassFileParser via constructor (no default instantiation) - Uses ClassFileParser for file scanning instead of its own RecursiveDirectoryIterator + RegexIterator - Application::discoverPlugins() simplified to iterate definitions and register
279e2ad to
cf989f4
Compare
Plugin/PreferenceDiscovery were taking ClassFileParser through their constructors, but the parser is a stateless utility with one implementation that's never mocked. Constructor injection here was ceremony — every test had to construct a real ClassFileParser inline just to satisfy the signature. Both classes now `new ClassFileParser()` once inside discoverInModule(). Application no longer threads $this->classFileParser through to either discovery class. Restored the str_contains pre-filter in PluginDiscovery so non-plugin files don't pay for extractClassName + loadClass + reflection. Dropped the redundant class_exists() check after loadClass() (already guaranteed). For consistency, applied the same cleanup to PreferenceDiscovery that was the subject of the prior refactor in marko-php#44. Co-Authored-By: Lasse Larsen <iamlasse@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks again @iamlasse — clean, well-framed PR this time, and the structural intent matches the follow-up I'd called out in #44. Pushed a follow-up commit ( Kept and validated
Adjusted
Constructor cleanup (and consistency with #44)
Tests pass (4845), lint clean. Ready to merge. |
What
Offloads plugin discovery logic into
PluginDiscoverysoApplicationdoesn't duplicate the work. Follow-up to #44.Also drops the
ClassFileParserconstructor injection that #44 introduced onPreferenceDiscovery— see "Constructor cleanup" below.Changes
PluginDiscovery::discoverInModule()returnsPluginDefinition[](already-parsed) instead of raw file paths.loadClass+ reflection moves out ofApplication::discoverPlugins()intoPluginDiscovery.PluginDiscoverykeeps the cheapstr_contains('#[Plugin')pre-filter so non-plugin files don't pay for extraction + autoload + reflection.Application::discoverPlugins()simplified to iterate definitions and register.Constructor cleanup
PluginDiscoveryandPreferenceDiscoveryno longer takeClassFileParservia constructor — theynewone internally. The parser is a stateless utility class with one implementation that's never mocked, so the injection was ceremony: tests had to construct a parser inline just to satisfy the signature. Removing it cuts test boilerplate and avoids a public-API break for anyone who was instantiating these directly.Applicationno longer threads$this->classFileParserthrough to either discovery class. The property remains forCommandDiscovery, which still uses it.