Closed phptek closed 5 years ago
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.
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 ;-)
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));
}
}
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!
@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?
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
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 :-)
@robbieaverill we tried targeting master but it included a bunch of unrelated commits. We've targeted the 4
branch instead
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 🤦♂️
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. :)
I like it.
Note that there's a PR already at https://github.com/silverstripe/silverstripe-framework/pull/8731
Closing as a duplicate of https://github.com/silverstripe/silverstripe-framework/issues/6329
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 aDataObject
into JSON viaJSONDataFormatter
which makes use ofViewableData::obj()
when casting.This occurs becuase of the core method
DataObject::data()
which is "blindly" called byViewableData::obj()
instead of the desiredDBField
bySilverStripe\Core\CustomMethods::hasMethod()
.The problem can be demo'd fairly succinclty below:
Steps to Reproduce
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 anException
ondev/build
when attempting to declare a field-name that is reserved. The reserved field-names could be declared as an immutible array inDataObject
to permit further fields to be added by core-devs in a flexible manner going forward.Related PRs