[ORM] Database field names are not guarded against ORM method clashes

[padbor]

Consider this:

<?php
class book extends DataObject {

    private static $db = array( 
        "Show" => "Boolean",
        "Delete" => "Boolean"
    );

    private static $summary_fields = array (
        "Delete" => "Delete",
        "Show" => "Show"
    );

}

Each time the administration is reloaded, the data for that model is removed from the database.

This removed more than 300 documents from my database.

This is because SilverStripe calls the Delete() method in $summary_fields

Which in this case is a very very sensitive method.

In some cases, this little trick of SilverStripe can go very fast and save lives.

But destroys data now. And it took me 48 hours and think that I was attacking, siphoning ...

For example, you can update the Delete method of the DataObject model to:

function delete ($now = false) {
    If ($now) {
      $this->brokenOnDelete = true;
       $ this->onBeforeDelete();
   }
 }

So to call the Delete method, $now must take true, so that the data is deleted.

PRs

• https://github.com/silverstripe/silverstripe-framework/pull/9549

[tractorcow]

Alright, so what we need is a better check for reserved keys for properties, similar to how we do for form actions. What you would expect is that framework would crash with a "Invalid property name" error right?

E.g. a property Delete should fail if there is a method delete on the parent record (case insensitive).

[tractorcow]

I suggest to put the fix in DataObject::db() method (or DataObjectSchema::fieldSpec method in 4.x).

To improve performance, maybe the check should only be in dev mode.

[dhensby]

The cause of this is that GridField@getDataFieldValue calls $record->$fieldName(), which it probably shouldn't do - it should stick to getting DB values not invoking methods.

I think reserving all method names from being DB names is a bit much and surely we'd have to also reserve all the controller methods and all methods on the class hierarchy of both model and controllers too? Oh and decorators, then any GlobalTemplateProviders and so on, which all seems unnecessary and impractical.

What happens if I add a method later with the same name or I install a new module which causes a clash? Who's wrong, the method or the DB field?

Brutally honest mode engaged:

1. Naming a field "Delete" or "Write" or any word that's such a core part of the Model's API is going to cause problems
2. This would come up pretty quickly in testing and deleting 300 rows from a test DB shouldn't be a big deal
   • if you didn't test, then your backups will be useful here
   • if you didn't back up then you're just scapegoating the framework for the fact you didn't put in any safeguards against you deploying untested code.

What scenario is it even needed to store a boolean on an object called "delete"?

[tractorcow]

I would say:

private static $db = [
    'Name' => 'Type',
];

Would accept getName() as a method, but it should fail if Name is a method on the parent object. DB getters should use the property getters, not the non-getter form of these fields.

Unlike relation names, however, we can't easily guard against these. However, instead we can fail on names that clash with a PARENT class. E.g. function Children() would be fine with a has_one => Children, but it should be a validation error if the function Children() is in a parent class. This would cover relations named delete and so on in subclasses of DataObject.

These errors would be checked on dev/build.

Does this sound reasonable?

[dhensby]

gah - I'd just tell people to be sensible with their naming ;)

but yes, that sounds reasonable

[elliot-sawyer]

Does this imply that destructive methods like delete can be triggered with a GET request?

[jakxnz]

Is https://github.com/silverstripe/silverstripe-framework/pull/9549 a reasonable solution @dhensby ?