otter icon indicating copy to clipboard operation
otter copied to clipboard

Field Types: Make object oriented, support custom field types.

Open earboxer opened this issue 5 years ago • 5 comments

Currently, field types are defined by simple strings. This makes it easy to define a list of attributes, but lets say I wanted some more custom field types

class User extends OtterResource
{
    public function fields() {
        return [
            'name' => 'text',
            'password' => 'password',
            'email' => 'email',
            'purpose_statement' => 'textarea',
            'biography' => 'textarea',
            'favorite_color' => 'color',
        ];
    }
...

Currently, https://github.com/poowf/otter/pull/43 implements the textarea by changing FormComponent.vue, SingleResourceComponent.vue, and TableComponent.vue. While this works for the previous example, it's hard to customize.

For example, if I wanted the table view of 'favorite_color' to just show the favorite color (instead of the string #000000, I would need to customize FormComponent.vue to have another if statement in it. If I wanted to increase the number of rows in the textarea element for biography, I would need to define a textarea-longer type, and write if statements to make sure those got carried over.

Proposal: Create an Object Interface defining rendering for the listing, viewing, and editing interfaces. Allow parameters to be set for each one. (Also allow these to define defaults for 'hidden' and 'validations')

Possible usage

    public function fieldTypes() {
        return [
            'name' => [BasicField::class, ['type' => 'text'],],
            'password' => PasswordField::class,
            'email' => [BasicField::class, ['type' => 'email'],],
            'purpose_statement' => TextAreaField::class,
            'biography' => [TextAreaField::class, ['rows' => 20],],
            'favorite_color' => ColorField::class,
        ];

earboxer avatar Apr 13 '19 13:04 earboxer

Possible implementation of interface

interface FieldTypeInterface
{
    public function __construct(array $params);
    /**
     * @brief render functions take the object and the key and return something Renderable.
     */
    public function renderEdit($object,$key,$value);
    public function renderList($object,$key,$value);
    public function renderView($object,$key,$value);
    public function validations() : array;
}

Questions to answer

  • Should we be passing the object to this function, or the value from the object?
    • Not passing the object will force developers to write more code in their models
  • How should we treat fields types that use multiple columns from the model?
    • 'lat' => [LocationField::class, ['latitudeKey' => 'lat', 'longitudeKey' => 'lon']] -- and having renderEdit() create fields with name='lat' and name='lon'
    • after defining getLatLonAttribute and setLatLonAttribute in eloquent model, 'lat_lon' => LocationField::class,
    • 'lat,lon' => LocationField::class -- "native"-looking support for multiple fields

earboxer avatar Apr 13 '19 14:04 earboxer

This seems like a breaking change, since hidden() wouldn't really be needed (just have the fieldType return nothing for renderList and renderView).

Changelog for users

  • Deprecate fields() -- write it to automatically use BasicField and PasswordField so most people's existing OtterResources would still work fine.
  • Create fieldTypes() accepts an array of key => classname, key => [classname], key => [classname, [optionA => valueA]]
  • hidden() is deprecated -- does nothing (users are encouraged to pass 'hidden' => TRUE as parameters to field types that use it)
  • validations() now have default values for some field types. They can of course be overwritten (and should be in most cases)

Do you think it would be worth it to provide some kind of support for hidden() so that it continues to work? This would depend on the number of production systems running otter and the number of hidden fields that they use (other than password). If this project had version numbers (using semantic versioning), we would want to bump up the version number.

earboxer avatar Apr 13 '19 14:04 earboxer

I think we can work with this and target v1.0.0 since I have not actually versioned it to anything at the moment. This package is still in it's infancy so I think breaking changes.. would be a given. I'm not too sure about the usage of Otter in production as I don't have any tracking of sorts but if we base the amount of users off the statistics from packagist, I think we're good to make the breaking change for a better modular package.

zanechua avatar May 26 '19 19:05 zanechua

Thinking more about this: passing them as objects shouldn't hurt anything, performance-wise, we're only ELV-ing one thing at a time anyway. Also, then we can just allow each one to define its own constructor! Hooray!

      return [
            'name' => new BasicField('text'),
            'password' => new PasswordField(),
            'email' => BasicField('email'),
            'purpose_statement' => TextAreaField(),
            'biography' => TextAreaField(['rows' => 20]),
            'favorite_color' => ColorField(),

And looking at my FieldTypeInterface from above, I'm not sure why I wanted to pass object, key, and value. Presumably $value is always equal to $object->$key. It would lead to inconsistencies if we did it some other way...

This leaves us still without a way of getting two keys. I think the 'lat,lon' idea ~~is~~(could be) the best. (if we use the fields to optimize the query, we'd also have to split this there, but I think the getLatLonAttribute way provides the same kind of challenge... maybe optimizing queries isn't a goal of this project).

Personally, I'm not too concerned with validations (I just don't know that I'd use it), so I won't make major suggestions for change here (so, maybe I'll accidentally break it on implementation). Architecturally, it seems like something that should be moved inside renderEdit (at least for the client-side).

earboxer avatar Feb 22 '20 06:02 earboxer

A few concerns with allowing or recommending the use of non-fields as keys:

They can't be used in select or where (which we may want to improve the search box in the future (FELV-ing*?)) or orderBy (we'll want server-side sorting).

* Facet, Edit, List, View

For faceting, I think we would want to allow the search box to modify the model query that is happening with 1 or more where statements.

e.g. lat > 42.8 could generate ->where('lat','>', 42.8)

Different fields might want to define their own verbs: lat,lon within 87G7PX00+ could generate ->where('lat', '>', 40.7)->where('lat','<',40.75)->where('lon','>'-74.05')->where('lon','<',40.75)...

Maybe that's pushing it too far (we could use leverage eloquent query scopes for some of this).

That being said, I think enough of the thinking about this is done for it to start.

earboxer avatar Feb 23 '20 00:02 earboxer