Make extension hook methods protected

GuySartorelli commented 2 years ago

https://github.com/silverstripe/silverstripe-framework/pull/10457 made it so that extension hook methods can be protected and won't be exposed as callable methods on the object being extended. It should now be considered best practice to make hook methods protected - the only methods which should be public are methods that are intended to be called directly on the object being extended.

e.g. you wouldn't call $record->updateCMSFields() but you might call $record->getCustomField(). So updateCMSFields() should be protected and getCustomField() would be public on the Extension class.

Acceptance Criteria

[ ] Any extensions declared in core code and supported module use protected methods for their extension hook methods
[ ] Methods which should be called directly on the extended record are still public
[ ] Documentation examples of extension hook methods use protected method signatures

Follow up

We may want to discuss doing more changes to extension beyond that. But for now we'll only look at making methods protected.

PRs

DOC PRs

michalkleiner commented 2 years ago

Generally I'm in favour of this change, I just can see how someone would be calling now-public methods we think should be protected. Even with everything documented in a changelog I just can't foresee how big an issue that might be, but I guess if someone does that and won't know how to move away from that during a CMS5 upgrade then we can possibly say they're doing something wrong anyway — is that enough of an excuse?

GuySartorelli commented 2 years ago

Extension hook methods aren't meant to be called directly - if someone's doing that then yeah I think it's okay to say they're doing something wrong and should call $record->extend() instead.

maxime-rainville commented 1 year ago

If this gets done in time for the beta, it can be in CMS5. Otherwise it will have to wait for CMS6.

maxime-rainville commented 4 months ago

I just moved this in refinement. Are there other aspect we think should be refactored?

I've always thought $this->owner should be private and people should use the getOwner() instead.
I vaguely remember a rent from @GuySartorelli about DataExtension provided no value over Extension.
What about strongly typing more or all of Extension?

GuySartorelli commented 4 months ago

I've always thought $this->owner should be private and people should use the getOwner() instead.

Fully agree - though we might want to have an early idea of how many of these sorts of changes we're looking to make.

This is the sort of change that on its own wouldn't be too painful in an upgrade, but with a few others like it you start to want a tool to assist you in finding and replacing things.

I vaguely remember a rent from @GuySartorelli about DataExtension provided no value over Extension.

This issue deals with deprecating DataExtension

What about strongly typing more or all of Extension?

Seems like it'd be easy enough, though perhaps low value since Extension really only has getOwner() as far as methods that are actively called by developers, and we can't give that a type other than I guess object which isn't overly helpful.

emteknetnz commented 4 months ago

I don't really agree with changing ->owner - searching for ->owner-> in sink a quick string search found 720 matches. Changing it's pretty trivial, just find and replace in files and lots of pull-requests. However loads and loads of projects will use ->owner, personally I prefer it because it's less typing. I think that's just adding upgrade friction for basically zero benefit.

emteknetnz commented 4 months ago

I made a rough script for finding implementations of extension hooks, it's not 100% though I bashed it out in less than an hour, easy enough to get it to do everything we need to automate most of it - https://github.com/emteknetnz/extension-protector/blob/main/run.php

emteknetnz commented 4 months ago

I've put this into peer review to discuss what should be done with the list of $doNotMakeHookProtected in the extension-protector script I create for this issue

Options:

Do nothing and leave the Public. Proceed with making extensions methods in $makeHookProtected
Refactoring in CMS 6 to make it so those extension hooks are not directly called any more, would spawn several new cards. Would proceed with 1 in the meantime.
Do not proceed, leave everything public. Close this card.

GuySartorelli commented 4 months ago

It looks like 2 is actually "1, and create cards to consider doing 2 later" which is what I'd opt for.

GuySartorelli commented 4 months ago

@emteknetnz The ACs talk about updating docs but there's no docs PR - were no changes necessary?

GuySartorelli commented 4 months ago

Also quite a few with CI still failing after merging the framework PR. Will hold off merging any in case there are larger sweeping changes needed.

GuySartorelli commented 4 months ago

Still some failing CI, e.g.

GuySartorelli commented 4 months ago

Just one PR left with failing CI https://github.com/silverstripe/silverstripe-subsites/pull/571

GuySartorelli commented 4 months ago

PRs merged. Assigning to Steve to make the new card discussed in https://github.com/silverstripe/silverstripe-framework/issues/10514#issuecomment-2113703521 for the items that weren't straight forward.

Once that card exists, this issue can be closed.

emteknetnz commented 4 months ago

Created a new card https://github.com/silverstripe/silverstripe-framework/issues/11258

Created a new PR for changelog https://github.com/silverstripe/developer-docs/pull/520

GuySartorelli commented 4 months ago

PR merged

silverstripe / silverstripe-framework