Make extension hook methods protected

[GuySartorelli]

#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

• API Don't publicly expose extension hook methods #10515
• API Set extension hook implementation visibility to protected #11245 << merge first and then rerun CI on others
• API Set extension hook implementation visibility to protected #11252 << merge and then rerun CI on others
• API Set extension hook implementation visibility to protected bringyourownideas/silverstripe-composer-update-checker#86
• API Set extension hook implementation visibility to protected bringyourownideas/silverstripe-maintenance#223
• API Set extension hook implementation visibility to protected silverstripe-elemental#1190
• API Set extension hook implementation visibility to protected silverstripe-admin#1749
• API Set extension hook implementation visibility to protected silverstripe-asset-admin#1461
• API Set extension hook implementation visibility to protected silverstripe-assets#605
• API Set extension hook implementation visibility to protected silverstripe-auditor#79
• API Set extension hook implementation visibility to protected silverstripe-blog#757
• API Set extension hook implementation visibility to protected silverstripe-campaign-admin#305
• API Set extension hook implementation visibility to protected silverstripe-cms#2942
• API Set extension hook implementation visibility to protected silverstripe-contentreview#233
• API Set extension hook implementation visibility to protected silverstripe-documentconverter#79
• API Set extension hook implementation visibility to protected silverstripe-errorpage#107
• API Set extension hook implementation visibility to protected silverstripe-frameworktest#177
• API Set extension hook implementation visibility to protected silverstripe-graphql#586
• API Set extension hook implementation visibility to protected silverstripe-gridfieldqueuedexport#106
• API Set extension hook implementation visibility to protected silverstripe-ldap#76
• API Set extension hook implementation visibility to protected silverstripe-linkfield#287
• API Set extension hook implementation visibility to protected silverstripe-login-forms#177
• API Set extension hook implementation visibility to protected silverstripe-lumberjack#148
• API Set extension hook implementation visibility to protected silverstripe-mfa#540
• API Set extension hook implementation visibility to protected silverstripe-restfulserver#124
• API Set extension hook implementation visibility to protected silverstripe-session-manager#194
• API Set extension hook implementation visibility to protected silverstripe-sharedraftcontent#234
• API Set extension hook implementation visibility to protected silverstripe-sitewidecontent-report#87
• API Set extension hook implementation visibility to protected silverstripe-spamprotection#116
• API Set extension hook implementation visibility to protected silverstripe-staticpublishqueue#189
• API Set extension hook implementation visibility to protected silverstripe-subsites#571
• API Set extension hook implementation visibility to protected silverstripe-taxonomy#113
• API Set extension hook implementation visibility to protected silverstripe-textextraction#93
• API Set extension hook implementation visibility to protected silverstripe-userforms#1288
• API Set extension hook implementation visibility to protected silverstripe-versioned#445
• API Set extension hook implementation visibility to protected silverstripe-versioned-admin#340
• API Set extension hook implementation visibility to protected silverstripe-versionfeed#108
• API Set extension hook implementation visibility to protected symbiote/silverstripe-advancedworkflow#531
• API Set extension hook implementation visibility to protected symbiote/silverstripe-gridfieldextensions#391
• API Set extension hook implementation visibility to protected symbiote/silverstripe-queuedjobs#427
• API Set extension hook implementation visibility to protected tractorcow-farm/silverstripe-fluent#850
• API Set extension hook implementation visibility to protected silverstripe-sitewidecontent-report#89
• API Set extension hook implementation visibility to protected tractorcow-farm/silverstripe-fluent#852

DOC PRs

• DOC Update extension hook examples to be protected silverstripe-elemental#1194
• DOC Update extension hook examples to be protected developer-docs#519
• DOC Update extension hook examples to be protected silverstripe-ldap#78
• DOC Update extension hook examples to be protected silverstripe-mfa#542
• DOC Update extension hook examples to be protected silverstripe-restfulserver#125
• DOC Update extension hook examples to be protected silverstripe-totp-authenticator#158
• DOC Update extension hook examples to be protected symbiote/silverstripe-queuedjobs#429
• DOC Protected extension hook implementations in changelog developer-docs#520

[michalkleiner]

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]

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]

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

[maxime-rainville]

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]

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]

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]

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]

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:

1. Do nothing and leave the Public. Proceed with making extensions methods in $makeHookProtected
2. 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.
3. Do not proceed, leave everything public. Close this card.

[GuySartorelli]

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

[GuySartorelli]

Still some failing CI, e.g.

• https://github.com/silverstripe/silverstripe-staticpublishqueue/actions/runs/9107096785/job/25201304222?pr=189
• https://github.com/tractorcow-farm/silverstripe-fluent/actions/runs/9153805422/job/25201325256?pr=850

[GuySartorelli]

Just one PR left with failing CI
silverstripe/silverstripe-subsites#571

[GuySartorelli]

PRs merged.
Assigning to Steve to make the new card discussed in #10514 (comment) for the items that weren't straight forward.

Once that card exists, this issue can be closed.

[emteknetnz]

Created a new card
#11258

Created a new PR for changelog
silverstripe/developer-docs#520

[GuySartorelli]

PR merged