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

NEW DBField validation #11397

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Sep 23, 2024

Issue #11391

To share the email validation logic between the EmailField::validate() and DBEmail::validate() I've simply used the symfony email constraint

There are now 3x different validate() methods

  • FormField::validate(Validator $validator): bool
  • DataObject::validate(): ValidationResult
  • DBField::validate(): ValidationResult [new]

Validator should really be renamed to FormValidator for clarify. A (Form)Validator has a ValidationResult as a property

TODO:

  • When intantiationing EmailField, set a flag if there's an associated DBField (connected to DataObject, not just an arbitary "ModelField"), and if so, skip EmailField::validate()
  • private static $required on DataObject and validate their existance DataObject::validate(). No need to use RequireFields()/CompositeValidators for this. Should deprecate possibly RequiredFields when used in DataObject::getCMSCompositeValidator(), or just deprecate DataObject::getCMSCompositeValidator() entirely

@emteknetnz emteknetnz mentioned this pull request Sep 23, 2024
2 tasks
$validator->validationError(
$this->name,
_t('SilverStripe\\Forms\\EmailField.VALIDATION', 'Please enter an email address'),
'validation'
$validationResult->getMessages()[0]['message'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the message from the $validationResult rather than simply using $message in case we want to use the special symfony strings such as {{ value }} in the future

return $result;
}

public function scaffoldFormField(?string $title = null, array $params = []): ?FormField
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copy pasted from DBVarchar, just changing the TextField to an EmailField

@@ -43,6 +45,10 @@
*/
abstract class DBField extends ModelData implements DBIndexable
{
public function validate(): ValidationResult
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signature matches DataObject::validate() for consistency

This method should be moved further down the class

@emteknetnz emteknetnz force-pushed the pulls/6/dbfield-validation branch 4 times, most recently from 98198a5 to 2bf5c61 Compare September 24, 2024 07:30
@emteknetnz emteknetnz force-pushed the pulls/6/dbfield-validation branch from 2bf5c61 to fe46daa Compare September 24, 2024 07:45
@emteknetnz
Copy link
Member Author

Closed in favor of #11402

@emteknetnz emteknetnz closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant