REFACTOR: Remove getType() override to allow extensibility#33
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() override and instead leveraging the parent BaseElement::getType() implementation which uses configurable properties. This allows sites to customize the element's display name through extensions by simply setting $singular_name without needing to override methods.
- Removed the
getType()method override that returned a hardcoded translated string - Added
$singular_name = 'Media'and$plural_name = 'Media Blocks'configuration properties - Maintains backward compatibility with existing translations while enabling extensibility
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @var string | ||
| */ | ||
| private static $plural_name = 'Media Blocks'; |
There was a problem hiding this comment.
The plural name 'Media Blocks' is inconsistent with the existing translation in lang/en.yml which uses 'Media Elements' (PLURALNAME: Media Elements). Consider using 'Media Elements' to maintain consistency with existing translations, or update the translation file accordingly.
| private static $plural_name = 'Media Blocks'; | |
| private static $plural_name = 'Media Elements'; |
| /** | ||
| * @var string | ||
| */ | ||
| private static $singular_name = 'Media'; |
There was a problem hiding this comment.
The singular name 'Media' differs from the existing translation in lang/en.yml which uses 'Media Element' (SINGULARNAME: Media Element). While 'Media' matches the old getType() return value, consider whether 'Media Element' would be more consistent with the existing translations and provide better clarity in the CMS.
| private static $singular_name = 'Media'; | |
| private static $singular_name = 'Media Element'; |
| */ | ||
| private static $icon = 'font-icon-block-media'; | ||
|
|
||
| /** |
There was a problem hiding this comment.
[nitpick] Consider adding a docblock comment explaining that this value is used as the fallback for getType() when no translation is available, and that it allows customization via extensions. This would help future maintainers understand the purpose of this configuration property.
Example:
/**
* Singular name for this element type.
* Used as fallback by getType() when BlockType translation is not available.
* Can be customized via extensions to change the element's display name.
*
* @var string
*/| /** | |
| /** | |
| * Singular name for this element type. | |
| * Used as fallback by getType() when BlockType translation is not available. | |
| * Can be customized via extensions to change the element's display name. | |
| * |
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