Naming DBField as "Data" confuses core

[phptek]

Affected Version

framework 4.1.0 / PHP7.0

Description

Creatiing a simple DataObject with a single Text field and naming it "Data" will cause problems when Calling $this->obj('Data') in any context. In my case, I am using the restfulserver package which attempts to decompose a DataObject into JSON via JSONDataFormatter which makes use of ViewableData::obj() when casting.

This occurs becuase of the core method DataObject::data() which is "blindly" called by ViewableData::obj() instead of the desired DBField by SilverStripe\Core\CustomMethods::hasMethod().

The problem can be demo'd fairly succinclty below:

Steps to Reproduce

class Page extends SiteTree
{
    private static $db = [
         'Data' => 'Text''
    ];

    public function getCMSFields()
    {
        var_dump($this->obj('Data')); // prints `SiteTree` not `DBText`
        die;
    }

Workaround

The workaround is to declare a method on your datamodel named for the field and call getField('Data') from it, thus inverting otherwise default control.

Suggestions

"Data" is used in a DataObject context as DataObject::data() and is therefore a reserved word. As such, userland logic could receive an Exception on dev/build when attempting to declare a field-name that is reserved. The reserved field-names could be declared as an immutible array in DataObject to permit further fields to be added by core-devs in a flexible manner going forward.

Related PRs

• NEW: Reserved field names for Data Object #8731

[maxime-rainville]

I like the idea of throwing an Exception dev/build if a DataObject uses a reserve keyword as a field name. If we're going to do this, it probably be worthwhile to identify further reserve keywords that might casue issues.

[phptek]

Sure, or just add a single one to the list for now given the frameworks' 10+ year history without any similar issue (AFAIK) otherwise we might be waiting a while ;-)

[elliot-sawyer]

Suggested code-snippet from @phptek to help out the academy students:

public function requireDefaultRecords()
{
	...
	...
    // Protect devs from falling into the trap of naming a DB field after a reserved word
    foreach ($this->getSchema()->databaseFields($this->getClassName()) as $name => $field) {
        if (in_array($name, $this->config()->get('reserved_field_names'))) {
            throw new Exception(sprintf('%s is a reserved field name', $name));
        }
    }

[robbieaverill]

I think adding a new exception might constitute a major change in semver. If you're happy to make the PR against the master branch for SS5 then that sounds OK to me!

[phptek]

@robbieaverill interesting. Is throwing an exception in any area of the dev/build pipeline considered a major change?

What we could do instead is simply echo a warning to the CLI / GUI during dev/build. but not explicitly halt execution. Then again, dev's may quite easily miss that - but is that better than the status quo?

[robbieaverill]

We’ve “avoided” this issue in the past by issuing a user_warning instead. Having said that, you’re defensively coding against changes that won’t work in practice anyway, so we could probably justify an exception here as a minor semver change

[phptek]

I thought I read somewhere that SilverStripe (devs) are really looking to move away from user_error() and trigger_error() etc, for precisely the reason you would want to use an Exception.

And yeah, you're bang-on wr/t this being a change to something that is already "broken". Nice one. I'll leave it to the academy devs then :-)

[elliot-sawyer]

@robbieaverill we tried targeting master but it included a bunch of unrelated commits. We've targeted the 4 branch instead

[kinglozzer]

Another one to add to the list: Author seems to have issues in some scenarios where versioning is enabled, I just can’t for the life of me figure out why...

Edit: because of Versioned::Author(), obviously... I need a coffee 🤦‍♂️

[tractorcow]

Couldn't the exception be whenever the dbfield matches a method on that class? E.g.

public function requireDefaultRecords()
{
	...
	...
    // Protect devs from falling into the trap of naming a DB field after a reserved word
    foreach ($this->getSchema()->databaseFields($this->getClassName()) as $name => $field) {
        if (method_exists($this, $name)) {
            throw new Exception(sprintf('%s cannot be both a db field and a method', $name));
        }
    }

This still allows db fields to be overridden with getTheField / setTheField, but prevents conflicts between getTheField() and thefield()

In 5.x obviously due to the breaking change. :)

[phptek]

I like it.

[tractorcow]

Note that there's a PR already at #8731

[robbieaverill]

Closing as a duplicate of #6329