REFACTOR: Remove getType() override to allow extensibility#36
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the ElementOembed class to improve extensibility by removing the hardcoded getType() method override and instead relying on the base BaseElement::getType() implementation which uses i18n_singular_name(). This allows sites to customize the element's display name through extensions by simply setting the $singular_name property rather than overriding the entire method.
Key Changes
- Removed the
getType()method override that returned a hardcoded translation call - Added
$singular_name = 'Media'and$plural_name = 'Media Blocks'static properties to define default names
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Elements/ElementOembed.php
Outdated
| private static $singular_name = 'Media'; | ||
|
|
||
| /** | ||
| * @var string | ||
| */ | ||
| private static $plural_name = 'Media Blocks'; |
There was a problem hiding this comment.
The $singular_name and $plural_name values differ from the existing translation file values. The translation file (lang/en.yml) has SINGULARNAME: Media Element and PLURALNAME: Media Elements, but these are set to 'Media' and 'Media Blocks' respectively.
In SilverStripe, translation keys take precedence over static properties, so i18n_singular_name() will return 'Media Element' (not 'Media') and i18n_plural_name() will return 'Media Elements' (not 'Media Blocks'). This means the behavior may not match your expectations.
Consider either:
- Updating
lang/en.ymlto align with these new values, or - Changing these static properties to match the existing translation values
| private static $singular_name = 'Media'; | |
| /** | |
| * @var string | |
| */ | |
| private static $plural_name = 'Media Blocks'; | |
| private static $singular_name = 'Media Element'; | |
| /** | |
| * @var string | |
| */ | |
| private static $plural_name = 'Media Elements'; |
f623af9 to
3e4cddb
Compare
- Remove getType() method override so element inherits BaseElement::getType() - Add $singular_name = 'Media' and $plural_name = 'Media Blocks' - Allows extensions to customize display name via $singular_name Fixes #32 Related: dynamic/silverstripe-essentials-tools#68
3e4cddb to
b4b4e11
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Remove the
getType()method override so the element inheritsBaseElement::getType()which usesi18n_singular_name(). This allows sites to customize the element's display name via extensions by setting$singular_name.Changes
getType()method fromElementOembed$singular_name = 'Media'$plural_name = 'Media Blocks'Rationale
The base
BaseElement::getType()implementation already uses$this->i18n_singular_name():By removing the override and setting
$singular_name, extensions can now customize the display name in the CMS element picker by simply setting$singular_name, without needing to override the entire method.Fixes #32
Related: dynamic/silverstripe-essentials-tools#68