Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation in the cms does not properly validate form fields #1539

Closed
UndefinedOffset opened this issue Jul 31, 2023 · 3 comments
Closed

Validation in the cms does not properly validate form fields #1539

UndefinedOffset opened this issue Jul 31, 2023 · 3 comments

Comments

@UndefinedOffset
Copy link

UndefinedOffset commented Jul 31, 2023

In cases where there is no validator for a DataObject causing that DataObject to only lean on the CompositeValidator the validate method on FormField is not called unless you add another validator such as RequiredFields into the mix. The RequiredFields validator does not even have to have any fields flagged as required to cause the validate method to be called. For example an EmailField will not check to see if the value is actually an email address.

This appears to be a regression at some point because in Silverstripe 4.6 validation appears to be working properly on DataObjects in say a ModelAdmin, however in 4.13 (latest tags) and 5.0.0 (latest tags) the validation never happens. It seems to be that the issue was introduced in 4.13 on wards.

Acceptance criteria

  • In CMS Forms fields are automatically validated.
  • A new FieldValidator is created and automatically added to every CMS Form.

Note

  • The recaptcha fails when double validated. We might want to consider raising a PR to avoid this potential conflict
    Since we're only making this change for forms in the CMS, where nocaptcha isn't expected to be used, this should have no impact on that module.
  • While we'll probably add a new class to fix this, we feel comfortable shipping it in a patch release to address this very specific bug.

PRs

@GuySartorelli
Copy link
Member

Can you please provide a clear set of reproduction steps which, if followed, would result in working validation for CMS 4.6 but not working validation in 4.13?

@UndefinedOffset
Copy link
Author

Two part:

Custom object managed by a model admin

You need a simple data object such as:

<?php
use SilverStripe\Forms\EmailField;
use SilverStripe\ORM\DataObject;

class TestObject extends DataObject
{
    private static $db = [
        'Email' => 'Varchar(256)',
    ];

    private static $table_name = 'TestObject';


    /**
     * Gets fields used in the cms
     * @return \SilverStripe\Forms\FieldList Fields to be used
     */
    public function getCMSFields()
    {
        $fields = parent::getCMSFields();

        $fields->addFieldToTab('Root.Main', new EmailField('Email'));

        return $fields;
    }
}

And a model admin:

<?php
use SilverStripe\Admin\ModelAdmin;

class TestModelAdmin extends ModelAdmin
{
    private static $url_segment = 'test-admin';

    private static $managed_models = [
        TestObject::class,
    ];
}

When creating the object just put anything anything but a valid email address in the email field, note after attempting to save it will say that the email address is not valid (verified in a 4.5 install that I had kicking around). In Silverstripe 4.13 it will say nothing and happily save. The same thing happens in Silverstripe 5, it will just happily save. Only after you add a blank required fields will both 4.13 and 5 actually flag the field as invalid i.e:

/**
 * Gets the composite validator used in the cms
 * @return CompositeValidator Validator to be used
 */
public function getCMSCompositeValidator(): CompositeValidator
{
    $validator = parent::getCMSCompositeValidator();

    $validator->addValidator(
        new RequiredFields(
            // Note it's blank
        ),
    );

    return $validator;
}

On SiteConfig

Simply adding an EmailField for example to a SiteConfig extension will surface the issue here as well:

<?php
use SilverStripe\Forms\EmailField;
use SilverStripe\Forms\FieldList;
use SilverStripe\ORM\DataExtension;

class SiteConfigExtension extends DataExtension
{
    /**
     * Updates the CMS fields adding the fields defined in this extension
     * @param FieldList $fields Field List that new fields will be added to
     */
    public function updateCMSFields(FieldList $fields)
    {
        $fields->addFieldToTab('Root.Main', new EmailField('TestEmail'));
    }
}

Again only after adding a blank RequiredFields will it do any actual proper validation:

/**
 * Update composite validator to fix issue where the fields were not being validated properly
 * @param CompositeValidator $validator Validator to add to
 */
public function updateCMSCompositeValidator(CompositeValidator $validator): void
{
    $validator->addValidator(
        new RequiredFields(
            // Note it's blank
        )
    );
}

Hopefully that helps you reproduce the issue @GuySartorelli

@sabina-talipova
Copy link
Contributor

Issue was fixed. PRs merged. Close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants