Add extension hooks to update submitted form fields before processing

[michalkleiner]

Without this enhancement, there's no way how to easily tweak the existing or provide a custom field that will be used to receive the data when UDF is being processed.

[dhensby]

Looks to me that this function definition can be removed because it's defined in the parent class, no?

[michalkleiner]

They are different classes — FormField vs FileField

[michalkleiner]

Also, the extending class doesn't call the parent method so extending just the parent method would have no effect for the file field.

[dhensby]

My point is EditableFileField extends EditableFormField, so this duplicate declaration of getSubmittedFormField is not needed at all and can be completely removed from this class.

The parent can then be adjusted to use late static binding (static::create()) to replace $field = SubmittedFormField::create().

[michalkleiner]

I've renamed the extension point to be unique in each class, more specific in the file class, @dhensby.

I don't think we can use late static binding here. It is true EditableFileField extends EditableFormField, but they each instantiate a different class in their getSubmittedFormField method (SubmittedFormField vs SubmittedFileField) and those classes don't have any connection to the EditableForm/FileField classes using them in terms of inheritance.

[dhensby]

So what is the use-case here? I'm a bit unclear as to what kind of tweaks would be needed...

[michalkleiner]

We needed a custom behaviour for getFormattedValue and getLink methods of the SubmittedFileField class. Using Injector means you can't just extend the class and override the methods if the class also uses $db or $has_one (or any uninherited config stuff in general), which means duplication in the new class and possibly becoming out of sync with the original module.

Since we added the extension point for the submitted file field, it made sense to add the same hook into the parent class too to allow for updating of any field type.

[dhensby]

OK - I see, I think I'm getting a better understanding of your usecase here.

Wouldn't it be better to add extension hooks to SubmittedFileField than to getSubmittedFormField. I can only assume the way you'd plan to override with this hook is by attempting to return an entirely new object, which feels a bit wasteful if the hooks could be lower level.

This is the exact purpose of injector (and the SubmittedFileField::create() syntax), so adding a hook here so you can return a new object defeats the point of that syntax... And how are you overriding getFormattedValue and getLink, etc without extending SubmittedFileField and returning a whole new class. But if you're doing that it should absolutely be possible with injector...

So in a rambling kind of way I'm saying, this should be completely possible with injector.

[michalkleiner]

As mentioned before @dhensby, when replacing a class with Injector, it needs all $db, $has_one etc duplicated in the new class, since the Config collects these with the uninherited flag. Having NewSubmittedFileField extends SubmittedFileField and replacing it with injector isn't feasible because then you need to keep the new class in sync with the module's class manually around those aspects of it. Also it can't use the same table_name as that is tied to the original class, so data would need to be migrated.
I looked into all of the "more standard" approached, believe me. Adding extension points to the submitted file field class could potentially be possible, though it would need (in our case) 3 extension points and rewrite some of the methods since they use early returns. In the end this seemed to be the easiest way, not really complicating anything else for anyone else.

[dhensby]

Hi @michalkleiner - what you're saying is really concerning to me because it completely defeats the point of Injector and points to a much much bigger problem within the framework if what you're saying is the case. So I decided to investigate.

app/_config/udf.yml:

SilverStripe\Core\Injector\Injector:
  SilverStripe\UserForms\Model\Submission\SubmittedFileField:
    class: MySubmittedFileField

app/src/MySubmittedFileField.php:

<?php

namespace {

    use SilverStripe\ORM\FieldType\DBField;
    use SilverStripe\UserForms\Model\Submission\SubmittedFileField;

    class MySubmittedFileField extends SubmittedFileField {

        /**
         * Return the value of this field for inclusion into things such as
         * reports.
         *
         * @return string
         */
        public function getFormattedValue()
        {
            $name = $this->getFileName();
            $link = $this->getLink();
            $title = 'Extended Download File';

            if ($link) {
                return DBField::create_field('HTMLText', sprintf(
                    '%s - <a href="%s" target="_blank">%s</a>',
                    $name,
                    $link,
                    $title
                ));
            }

            return false;
        }
    }
}

I then created a UDF and just added a file field:

Then received this email:

The database doesn't create an extra table, data is stored in the original SubmittedFileField table.

So that seems to me like using Injector to replace the class works entirely as I would expect and means there is no need for your extension point.

[michalkleiner]

Thanks for giving it a shot, @dhensby. With your example above, if you now add a $db or $has_one entry to your MySubmittedFileField class, remove SubmittedFileField db table (or just wipe the whole db) and run dev/build, the SubmittedFileField table won't get created and MySubmittedFileField will get created instead, missing the fields from the SubmittedFileField class.

[dhensby]

Hi @michalkleiner - I've followed your instructions and indeed the table for SubmittedFileField is not created - which is really bad news and this is a more fundamental bug that we can't really just gloss over with a hook like this.

I think we need to open a bug report against the framework :(

[GuySartorelli]

Sounds like this PR is not a good solution for the problem. Closing in favor of silverstripe/silverstripe-framework#9952 which fixes the underlying problem.

[michalkleiner]

@GuySartorelli it wasn't meant to be a solution to the underlying problem, it was an enhancement supporting a workaround. I still see value in adding the extension points as an enhancement, unless there's a strong argument against them.

[GuySartorelli]

I guess my point is that there's no need to workaround it if that PR gets merged - there won't be anything to work around.
But if there is some use case other than working around that bug, or something I'm missing about the intention, I'm happy to reopen and review