Most DerivativeInterface implementations share common code(getDerivativeDefinition()
and data (protected $derivatives
).
Refactoring them to an abstract base class would help us remove some duplicated code and reduce the apparent complexity of plugin derivative creators by making them only have to write one method (getDerivativeDefinitions()
).
The already copy/pasted implementations are present in:
- BlockPluginUI
- CustomBlock
- DefaultWizardDeriver
- LanguageBlock
- Layout
- SelectionBase
- SystemMenuBlock
Alternate implementations are present in AggregatorCategoryBlock and AggregatorFeedBlock (more inline code), EntityDerivative (slightly different), and MockLayoutBlockDeriver and MockMenuBlockDeriver (always invoke getDerivativeDefinitions(), do not use $this->derivatives caching).
Comments
Comment #1
fgmTrying out patch.
Comment #2
fgmSlightly reworded for better clarity.
Comment #3
dawehnerJust some small points.
This should be "Contains \".
Let's use {@inheritdoc} instead
Comment #4
aspilicious CreditAttribution: aspilicious commentedDo we always need to say "implements derivativeInterface" if the base class already does it?
Lets add an empty implementation of this function in the base class
I started the same issue in the queue I'll close that one.
Comment #5
fgmMore simplifications, as suggested in #3 and #4.
The patches show two versions:
$this->derivatives
, then defer to the parent implementation to return. It is likely to be a little bit slower.Comment #7
dawehnerMicro-performance doesn't really matter, as plugin instances will be cached all the time.
Comment #8
aspilicious CreditAttribution: aspilicious commentedI would go with the first one. Needs review for that one.
Comment #9
fgmI would favour the first version too, but for the record, here is the second one in a single patch which the bot can test.
Comment #10
EclipseGc CreditAttribution: EclipseGc commented#5:1 looks good to me.
Eclipse
Comment #11
tim.plunkettLooks like this duplicated #1809200: Consolidate discovery code in a DiscoveryBase and DiscoveryDecoratorBase?
Oh well.
Comment #12
catchVery nice. Committed/pushed to 8.x, thanks!