API Strongly type return types #558
Conversation
emteknetnz
force-pushed
the
pulls/7/field-validators
branch
from
395c072
to
31f2df4
Compare
| // These are loosely typed to allow for easy conversion from YAML and backwards compatibility | ||
| // e.g. version is likely to be an int, though the DB column it goes into is a Varchar |
There was a problem hiding this comment.
| // These are loosely typed to allow for easy conversion from YAML and backwards compatibility | |
| // e.g. version is likely to be an int, though the DB column it goes into is a Varchar |
YAML is capable of type inference. I'm fine with not adding strong typing in this update, but I don't think we should include this comment as it implies we shouldn't add strong typing here at any point.
There was a problem hiding this comment.
I'm not sure why you mean by YAML is capable of type inference
The intention here is that developers are likely to define things with the wrong type in YAML, e.g. in workflows.yml we have - 0.2 which is used as a $version param in WorkflowTemplate::__construct(), ironically the default value of that param is '0.0'. The column it goes into is a varchar.
| public function getName() | ||
| public function getName(): string | ||
| { | ||
| return $this->name; | ||
| return (string) $this->name; | ||
| } | ||
|
|
||
| public function getVersion() | ||
| public function getVersion(): string | ||
| { | ||
| return $this->version; | ||
| return (string) $this->version; | ||
| } | ||
|
|
||
| public function getDescription() | ||
| public function getDescription(): string | ||
| { | ||
| return $this->description; | ||
| return (string) $this->description; | ||
| } | ||
|
|
||
| public function getRemindDays() | ||
| public function getRemindDays(): int | ||
| { | ||
| return $this->remindDays; | ||
| return (int) $this->remindDays; | ||
| } | ||
|
|
||
| public function getSort() | ||
| public function getSort(): int | ||
| { | ||
| return $this->sort; | ||
| return (int) $this->sort; | ||
| } |
There was a problem hiding this comment.
Why are we adding strong typing here, but not to the properties? Backwards compatibility doesn't seem like a sensible reason on its own, because strong typing is explicitly a breaking change - as as I mentioned above, YAML type inference is very much a thing.
This class isn't a DataObject and doesn't directly interact with DBField, so I'm struggling to see how this is related at all to the issue.
There was a problem hiding this comment.
As mentioned in the comment above, the properties need to be dynamic because it's likely that the types used in YAML are the wrong type, though we don't want people things to break. However we do need to cast to the correct type e.g. DBVarchar will fail validation if an int or float is returned for getVersion()
emteknetnz
force-pushed
the
pulls/7/field-validators
branch
from
31f2df4
to
6df3e54
Compare
|
In the interest of getting things merged I'll allow these changes. While I still don't think they're correct, they also don't seem harmful except in that they allow continued poor practice by developers. |
emteknetnz
force-pushed
the
pulls/7/field-validators
branch
from
6df3e54
to
5aab70f
Compare
Issue silverstripe/silverstripe-framework#11403