Inconsistent method visibility on hooks for DataObject's

Affected Version

All versions (as of writing)

Description

The methods onBeforeWrite, onAfterWrite, onBeforeDelete and onAfterDelete on DataObject have a logical visibility of protected, as they're intended to only ever be called internally. However, it appears to be common practice to override these methods with a visibility of public.

Why?

While this makes sense on DataExtension (due to how ->extend(...) invokes these methods), it does show an internal inconsistency in the typical API between DataObject itself and most descendants of that class. For example, objects like Member have ->onAfterDelete() hooks which would have a detrimental effect if they were ever called externally. While we don't expect folks to call this method out of context, I also think it doesn't make sense to allow these methods to stay exposed based on that expectation.

I'd suggest one of the following remedies:

Make all implementations of these hooks protected and write a unit test to enforce this change.
Make all implementations of these hooks public (no unit test needed as PHP will generate errors).

Also, optionally, DataExtention's could also be converted to protected visibility as well, but this would require ->extend() to leverage reflection in order to invoke those methods.

NOTE: This would obviously be a pretty big change to the API in either case, but I figured it was worth mentioning to see if there were some way to tackle this inconsistency (or determine if it were worth addressing at all).

Steps to Reproduce

Here's a quick script that will generate a full report on all classes/methods that are diverging from the visibility on the base DataObject class:

// Methods and their expected visibility.
$methods = [
    'onBeforeWrite' => 'protected',
    'onAfterWrite' => 'protected',
    'onBeforeDelete' => 'protected',
    'onAfterDelete' => 'protected',
    'onBeforeCMSWrite' => 'protected',
];

// Get all DataObjects to inspect now.
$manifest = SS_ClassLoader::instance()->getManifest(); // SS 3.x syntax
$allClasses = $manifest->getDescendantsOf('DataObject');
foreach($allClasses as $class) {
    // The expected methods are only populated if they exist but contains expected visibility.
    $expectMethods = [];
    $hasMethods = [];

    foreach($methods as $method => $expectViz) {
        // Works regardless of visibility.
        if (!method_exists($class, $method)) continue;

        $reflect = new ReflectionMethod($class, $method);
        $viz = 'public';
        if ($reflect->getDeclaringClass()->name !== $class) continue; // ... only notify on the classes where it's defined.
        if ($reflect->isProtected()) $viz = 'protected';
        if ($reflect->isPrivate()) $viz = 'private';

        // Only track the differences from expected visibility.
        if ($viz !== $expectViz) {
            $expectMethods[$method] = $expectViz;
            $hasMethods[$method] = $viz;
        }
    }

    if ($hasMethods !== $expectMethods) {
        echo "<pre><strong>$class</strong>\n";
        print_r($hasMethods);
        echo "\n\n</pre>";
    }
}

silverstripe / silverstripe-framework