Retroactive Trait compostion

[maxime-rainville]

We keep re-inventing the wheel and reusing the same patterns over and over. This causes code duplication, increases the risk of inconsistencies, leads to pointless arguments and make it more difficult to maintain our code.

FormField is an especially egregious example of this. If we had had a bit more foresight we could have use Trait composition to achieve the same results. Fortunately, there's nothing in semver (as far as I know) that prevents us from retroactively using traits as long as our method signatures stay consistent.

This would also make it easier for third party devs to re-implement those patterns for their own use cases. Might want to match all of this with intrefaces as well.

Possible traits

These could be good candidates for being extracted to traits.

HTMLAttributes

Allows an object meant to be rendered out as HTML to define attributes. This could be use on:

• FormField
• CMSMenuItem
• ImageManipulation.

Methods

• getAttributesHTML
• getAttributes
• getAttribute
• setAttribute

ExtraClass

Allows an object meant to be rendered out as HTML to manage CSS classes that should be applied to it. This could be use in:

• Form
• FormField

Methods

• setupDefaultClasses
• extraClass
• addExtraClass
• removeExtraClass
• hasClass

Template

Allows a ViewableData to manage its templates. This could be use in:

• Form
• FormField
• RSSFeed
• GridFieldDetailForm_ItemRequest

Methods

• getTemplate
• setTemplate
• getTemplates

Pull requests

• [x] https://github.com/silverstripe/silverstripe-framework/pull/10033 (Just the attributes HTML bit)

[maxime-rainville]

The Loaders and Manifest have the same problem.

Loader

Used by the kernel to load information about files in a manifest. This could be used in

• ClassLoader
• ModuleLoader

Methods

• inst
• getManifest
• hasManifest
• pushManifest
• popManifest
• countManifests
• init

Manifest

Used by the kernel or the a Loaders to find/register files. This could be used by:

• ClassManifest
• ModuleManifest
• ThemeManifest

Methods

• init
• regenerate

Properties

• $base
• $cacheKey
• $cacheFactory
• $cache

[dhensby]

be careful getting stuck into the manifests / class loaders. whilst it in theory has push and pop manifests and implied inheritence, in reality it doesn't actually work that way as I found to my peril.

An approach I wanted to take to the class manifest a while ago was to be able to "push" classes into it just for tests (so we could create a mock in PHPUnit and then push that manifest into the class loader so that it would then be available via Injector and so on... Nope, that doesn't work, because the manifests don't inherit as implied. Much time wasted and I realised that making it work as implied would also be a huge effort and probably break how things actually work at the moment

[maxime-rainville]

@dhensby The Manifest/Loader bit while working on https://github.com/silverstripe/silverstripe-framework/issues/10146

I agree there's not much value reinventing the manifest wheel. I do think it's worth pointing out that all the methods I've listed are literally copy-pasted across multiple files. So we are repeating the same pattern, without explicitly defining it as an interface or trait.

We don't provide a means for people to define their own manifest implementations. So if we did define a manifest trait/interface, it would only help core developers by making things a little bit clearer.