Problem/Motivation
Plugin discovery requires file directory scanning, loading classes into memory, then reflection to check for attributes.
We have to load the class into memory before we can check for plugin attributes, and this memory then can't be freed again.
Core has more than 100 classes that are in plugin directories, but are not plugins, or are different plugins.
For example
Convert.php
CreateNew.php
Crop.php
Desaturate.php
GDImageToolkitOperationBase.php
Resize.php
Rotate.php
ScaleAndCrop.php
Scale.php
These are ImageToolkitOperation operation plugins, which are in core/modules/system/src/Plugin/ImageToolkit/Operation/gd
But because they're in core/modules/system/src/Plugin/ImageToolkit they get parsed in case they're ImageToolKit plugins, this loads all the classes into memory and requires FileCache to store 'null' entries (which include mtime etc. in reality) in APCu.
If we move the GD toolkit operations into a parallel instead of nested directory, then we wouldn't be parsing them twice (or at all, when we only get the toolkits and not the operations).
There are various other cases, some base classes which will be annoying but probably worthwhile to move, some helper classes in Views it looks like.
Plugin discovery has explicit logic for when it discovers non-plugins so we can add an Exception there to start building a list.
Steps to reproduce
Proposed resolution
Move base classes/interfaces to src/Plugin
Move plugins to their own 'type' directory, src/Plugin/my_plugin_type/plugin_name.php
Remaining tasks
Move classes around
Update Plugin API documentataion
Comments
Comment #2
joachim commentedWe should define a standard for where plugin base classes and interfaces go.
Having these in the plugin folder is good DX. Having them dumped in top-level /src is messy.
I think @catch suggested /src/Plugin in slack?
Comment #3
catchYes I was thinking of moving them up one directory level into src/Plugin - keeps them close to the actual plugins, but not actually mixed in together then.
Comment #4
quietone commentedComment #6
godotislateThinking about this some more: if there are interfaces, traits, or base classes in a plugin directory, and if any plugin class in the same directory uses that interface/trait/base class, the performance gain might be marginal, since they would be loaded into memory anyway. They would take up additional entries in the file cache, though.
Comment #7
joachim commented> They would take up additional entries in the file cache, though.
Could we mitigate that by having discovery simply not look inside any file called *Trait, *Base, or *Interface? Don't even scan for an attribute, just skip it in the file finder stage.
I do like the DX of those classes being in the same folder as the plugins!
Comment #8
catchThat would work except for the specific case of #3502584: [PP-1] Remove nesting of ImageToolkitOperation plugins but that's pretty unique.
We have a coding standard that enforces that naming convention now, so it should be pretty reliable #3521443: Traits should always have the suffix "Trait".
Comment #9
godotislateWe have coding standard rules for Interface and Trait, but not Base. Is it completely safe to skip all classes that end in Base? Not sure.
Comment #10
catchThere's an issue here: #1567920: Naming standard for abstract/base classes but it's a bit stalled, and it at least partly got stalled on whether all base classes should be abstract which would involve code changes.
We could skip interfaces and traits for now, and then open an issue to skip base classes postponed on that issue?
Comment #11
godotislateIssues created:
#3574325: Skip parsing *Interface and *Trait files in plugin folders during attribute plugin discovery
#3574330: [PP-1] Skip parsing *Base files in plugin folders during attribute plugin discovery