Over the last few weeks I have been working with the Open Sourcerers team to develop a set of new features for SilverStripe 4.0. The goal is to implement the RFC-1 asset abstraction functionality, as presented by Mateusz at
https://github.com/silverstripe/silverstripe-framework/issues/3792
Just to give an indication of progress, we have already merged a prototype DBFile field into master, and am currently in the process of updating File dataobject to use this field, rather than itself being the manager for the physical underlying files.
Ok, so here's the problem I have run into, and am hoping that I could find help with.
Apart from FormField validation, it's also necessary to provide data-object level validation (triggered on write) to ensure that the underlying save of any file (whether to a DBFile field or a File dataobject) is saving the correct file type. This validation is extremely important when user code is relying on image content; Front-end templates sometimes rely on certain files being images, so that they can be resized, and we don't want to force template-level code to deal with that validation.
At the moment, the current status of the API looks similar to the below:
class File extends DataObject {
private static $db = array(
'File' => 'DBFile'
);
}
/** @deprecated */
class Image extends File{}
class DBFile extends DBField {
// contains code that used to be in File.php
}
class ExampleObject extends DataObject {
private static $db = array(
'BannerField' => 'DBFile'
);
private static $db = array(
'BannerObject' => 'File'
);
}
The issue with the current state of this API is that there is no way to force fields to be allowed as images only. Image ended up deprecated for the time being, in lieu of a solution for specifying a "DBFile which can only be an image".
A potential solution has been proposed, but would rely on certain other problems being solved. This API could look similar to the below:
class File extends DataObject {
private static $db = array(
'File' => 'DBFile'
);
}
class Image extends File{
public function __construct() {
$this->File->setAllowedCategories('image');
}
}
class DBFile extends DBField {
public function construct($types){
$this->types = $types;
}
public function validate() {
// some kind of error handling
}
}
class ExampleObject extends DataObject {
private static $db = array(
'BannerField' => 'DBFile(image)'
);
private static $db = array(
'BannerObject' => 'Image'
);
}
In this case, Image means "a file which can only be an image" and DBFile() or File means "a file of any type, including images".
The issue with this code is that as DBFile is a DBField, not a FormField, thus the DBFile::validate method is never called as a part of normal processing. Because the model exists at a lower level to the form subsystem, it's expected that this error will not be able to be run through the normal form validation, which surfaces errors to the user on the submitted form.
In this case, correct assignment of images to these fields rely on:
1. The user of this code creating an UploadField with the correct allowed file types.
2. If the user doesn't do this, then some mechanism exists to force validation to be checked on all DBFields prior to saving a dataobject.
It is necessary to ensure that, regardless of the formfield validation, the underlying data model can never have necessary constraints violated.
As it stands, DBFields such as SS_Datetime, will simply set "NULL" as the value if assigned an invalid value, without surfacing any notices.
There are a couple of directions we could take to ensure data consistency:
Option 1: maintain the status quo
DBFields will validate themselves on setValue, and set themselves to null if they do not have a valid value. If the user does NOT create a formfield with the necessary validation, this means that errors could be nullified without useful front-end errors. DBFile('image') would simply nullify any images assigned with extensions that aren't image types, as would the Image dataobject.
If the user sets up their formfields properly, errors will be properly displayed there before this validation step is reached.
We can still bake these rules into the necessary auto-scaffolding, and if a developer follows the same rules for generating custom fields, they should expect that they never run into fields being nullified on error.
Option 2: Hook DBField validation into DataObject::validate
On saving of dataobject, it will have to loop through each database field in order to validate itself.
class DataObject extends ViewableData {
public function validate() {
$result = ValidationResult::create();
foreach ($this->config()->db as $field => $spec) {
$field = $this->dbObject($field);
$field->validate($result);
}
$this->extend('validate', $result);
return $result;
}
}
This would sacrifice some performance, since we could potentially have many fields on a DataObject, and is quite a complicated solution to ensure DBFiles assigned directly to a DataObject are validated. There is also the risk that subclasses of DataObject could override this incorrectly, blocking validation on DBFields.
Option 3: Exception on invalid type
This is similar to option 1, but instead of nullifying, an exception would be generated.
This could cause disruption to workflow in a small number of cases, meaning errors will surface as CMS ajax popups rather than via the normal form validation (possibly). Throwing an exception (even if it's a ValidationException) does not guarantee it will be rendered as a part of normal form validation. However, this is the most robust for the sake of data validation, as invalid values can never be saved.
If user-code is diligent in the generation of their formfields, such that the correct file type filters are assigned to an UploadField, they should be able to avoid this. However, we could expect that a user could neglect to set these properly when not using the default form scaffolder.
To a degree, this is already a problem in the current 3.x, as users can create UploadFields for Image dataobject without setting the type validation rules.
Option 4: Something I haven't thought of that you have.
Fill in the blanks. :)
Of these solution, my preference is probably option 3. Does anyone else have a feeling on how this issue should be addressed?
Thanks for all your assistance. :)
Kind regards,
Damian Mooyman