Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate DataExtension and other superfluous classes #11050

Closed
emteknetnz opened this issue Nov 13, 2023 · 5 comments
Closed

Deprecate DataExtension and other superfluous classes #11050

emteknetnz opened this issue Nov 13, 2023 · 5 comments
Assignees
Milestone

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Nov 13, 2023

The convention has been to use DataExtension instead of Extension for DataObjects

DataExtension adds a bunch of extra methods and shouldn't be used, Extension should be used instead

Acceptance criteria

  • Deprecate DataExtension
  • Deprecate SiteTreeExtension
  • Deprecate LeftAndMainExtension
  • Deprecate any other abstract or abstract-like Extension subclasses
  • Message should be to subclass Extension directly instead
  • Remove documentation that refers to the deprecated extensions
  • Changelog indicates they have been deprecated
  • Any conditional logic that uses DataExtension::class or subclasses is identified and dealt with - for example

CMS 5 PRs

CMS 6 recipe-kitchen-sink multi PR CI

CMS 6 PRs - merge, merge-up and rebase CMS 5 PRs first

@maxime-rainville
Copy link
Contributor

Not sure about this one. The empty methods on [DataExtension](https://github.com/silverstripe/silverstripe-framework/blob/5/src/ORM/DataExtension.php) might not be necessary, but they have the value of making it clear what methods have special meaning when adding an Extension to a DataObject and enforcing their signature.

Minimally, updateSummaryFields and updateFieldLabels seem to be doing a little something.

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 13, 2024

they have the value of making it clear what methods have special meaning when adding an Extension to a DataObject

If we think that's important, we should have a policy to document them (perhaps updating the API docs to pick up on and document them automatically). Having empty classes to pick up the slack of documentation is an anti-pattern.

and enforcing their signature.

Enforcing their signature is actually bad for us, because it makes the method signature of extension hooks part of our public API that we can't alter outside of major releases and therefore we can't add new arguments to them.

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 20, 2024

DataExtension::updateFieldLabels() appears to do nothing, as it merges in Config::inst()->get(static::class, 'field_labels') (static::class does resolve to the owner class, not just DataExtension) with &$fields ... however that already happens in DataObject::fieldLabels() with $customLabels = $this->config()->get('field_labels'); ... $labels = array_merge((array)$autoLabels, (array)$customLabels); - $labels is an assoc array so the extension method is just duplicate code.

DataExtension::updateSummaryFields() is basically the same, it merges in fields of an assoc array from Config::inst()->get(static::class, 'summary_fields') ... however DataObject::summaryFields() already does this. There is some extra logic in the extension method to handle non assoc arrays, though based on my brief debugging it's always assoc arrays being passed in

These extension methods really do feel like old code that should have been removed a long time ago

This was referenced Aug 20, 2024
@GuySartorelli
Copy link
Member

CMS 5 PRs merged. Reassigning to Steve to rebase CMS 6 PRs

@GuySartorelli
Copy link
Member

PRs merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants