Skip to content

REFACTOR: Remove getType() override to allow extensibility (SS5)#34

Closed
jsirish wants to merge 2 commits into4from
fix/remove-gettype-override-ss5
Closed

REFACTOR: Remove getType() override to allow extensibility (SS5)#34
jsirish wants to merge 2 commits into4from
fix/remove-gettype-override-ss5

Conversation

@jsirish
Copy link
Copy Markdown
Member

@jsirish jsirish commented Dec 2, 2025

Summary

Remove the getType() method override so the element inherits BaseElement::getType() which uses i18n_singular_name(). This allows sites to customize the element's display name via extensions by setting $singular_name.

Changes

  • Removed the getType() method from ElementOembed
  • Added $singular_name = 'Media'
  • Added $plural_name = 'Media Blocks'

Rationale

The base BaseElement::getType() implementation already uses $this->i18n_singular_name():

public function getType()
{
    $default = $this->i18n_singular_name() ?: 'Block';
    return _t(static::class . '.BlockType', $default);
}

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

- Removed getType() method override
- Added $singular_name = 'Media'
- Added $plural_name = 'Media Blocks'

This allows sites to customize the element's display name via extensions
by setting $singular_name.

Fixes #32
Related: dynamic/silverstripe-essentials-tools#68
Copilot AI review requested due to automatic review settings December 2, 2025 07:42
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the ElementOembed class to improve extensibility by removing the hardcoded getType() method override. The change allows sites to customize the element's display name through extensions by simply modifying the $singular_name property, rather than needing to override the entire method. This aligns with the SilverStripe Elemental framework's design where BaseElement::getType() uses i18n_singular_name() for translation support.

Key Changes:

  • Removed the getType() method override that returned a hardcoded translated string
  • Added $singular_name = 'Media' to provide the default display name
  • Added $plural_name = 'Media Blocks' for plural display contexts

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

/**
* @var string
*/
private static $plural_name = 'Media Blocks';
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider consistency between $plural_name = 'Media Blocks' and the existing translation PLURALNAME: Media Elements in lang/en.yml. While these serve slightly different purposes, using consistent terminology ('Elements' vs 'Blocks') throughout the codebase may improve clarity. This is a minor naming preference issue.

Suggested change
private static $plural_name = 'Media Blocks';
private static $plural_name = 'Media Elements';

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 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.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 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.

@jsirish jsirish closed this Dec 2, 2025
@jsirish jsirish deleted the fix/remove-gettype-override-ss5 branch December 2, 2025 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants