getCMSCompositeValidator() is called on a singleton rather than the actual record in ModelAdmin.

[GuySartorelli]

Affected Version: 4.7.3

Description

Observed: Any functionality in the getCMSCompositeValidator method that requires actual data to exist on the record fails, because this method is called on a singleton rather than the true record.

Expected: Any functionality in the getCMSCompositeValidator method that requires actual data to exist on the record should work as expected - the method should be called on the actual record (otherwise it could just be a static method)

Steps to Reproduce

1: Create a subclass of DataObject and use some record value inside the getCMSCompositeValidator method, e.g.:

public function getCMSCompositeValidator(): CompositeValidator
{
    $validator = parent::getCMSCompositeValidator();
    if ($this->isInDB()) {
        die('this is not a singleton');
    }
    return $validator;
}

2: Create a subclass of ModelAdmin and with the class from step 1 in the $managed_models array. 3: Open the new admin section, and click the button to create a new record. The record-data-specific condition will not be met, so that code will not run.

Proposed solution:

Remove the below:

// Validation
if (singleton($this->modelClass)->hasMethod('getCMSCompositeValidator')) {
    $detailValidator = singleton($this->modelClass)->getCMSCompositeValidator();
    /** @var GridFieldDetailForm $detailform */
    $detailform = $config->getComponentByType(GridFieldDetailForm::class);
    $detailform->setValidator($detailValidator);
}

The GridFieldDetailForm already calls $this->setValidator($record->getCMSCompositeValidator()); if no validator has been previously set - and it uses the actual record rather than a singleton.

[GuySartorelli]

My current workaround for this is to override the getGridFieldConfig method in the ModelAdmin subclass as follows:

protected function getGridFieldConfig(): GridFieldConfig
{
    $config = parent::getGridFieldConfig();
    // Replace the GridFieldDetailForm with one that has no validator.
    $detailform = $config->getComponentByType(GridFieldDetailForm::class);
    $config->removeComponent($detailform);
    $config->addComponent(new GridFieldDetailForm(null, $detailform->getShowPagination(), $detailform->getShowAdd()));
    return $config;
}

[maxime-rainville]

It's not necessarily clear to me this behaviour is problematic ... if anything, it looks pretty sensible.

When your ModelAdmin form object is being built, it might not have a record attached to it yet. But it will still need its validator.

The Validator's php method receives all your submitted form data. So you should be able to use the submitted data to implement contextual validation rules.

In this context, having a "stateless" getCMSCompositeValidator method makes sense. Could you detail your exact use case a bit more?

[GuySartorelli]

When your ModelAdmin form object is being built, it might not have a record attached to it yet. But it will still need its validator.

The GridFieldDetailForm::handleItem method will get the instance if it exists, or else create a singleton at that stage. This means that when there is an actual record to use, if you haven't forced a validator onto the GridFieldDetailForm, it will fetch it from the actual existing record. If it doesn't exist, then a singleton is created by the GridFieldDetailForm but only if one doesn't exist. This precludes the necessity of doing this step in ModelAdmin and ensures that the validator is fetched from the actual instance whenever possible.

The Validator's php method receives all your submitted form data

Normally this is enough. I have a very unique situation however where I need to conditionally send a different set of validators inside the composite validator depending on some record-specific data. I won't go into exactly why I need to do this, but the system should be flexible enough to handle this case.

In this context, having a "stateless" getCMSCompositeValidator method makes sense

If the intention is that the method should always be 'stateless' why not make it static? I'm of the opinion that any instance method on a DataObject should be able to take advantage of real record data whenever a record exists.

All of that said, as mentioned there is a pretty easy workaround for this - so even if this ticket exists to help anyone else who might run into this hurdle find a way around it, that may be enough. It's an understandably very low priority.