Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions bundle/InfoCollection/Form/Extension/EmailFormExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php

declare(strict_types=1);

namespace Netgen\Bundle\SiteBundle\InfoCollection\Form\Extension;

use Ibexa\Contracts\ContentForms\Data\Content\FieldData;
use Netgen\Bundle\InformationCollectionBundle\Ibexa\ContentForms\InformationCollectionFieldType;
use Netgen\Bundle\InformationCollectionBundle\Ibexa\ContentForms\InformationCollectionType;
use Netgen\Bundle\SiteBundle\InfoCollection\Validator\Constraints\CustomEmail;
use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class EmailFormExtension extends AbstractTypeExtension
{
public static function getExtendedTypes(): iterable
{
return [InformationCollectionType::class];
}

public function configureOptions(OptionsResolver $resolver): void
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this configureOptions method, we're not translating anything here, besides validators domain is default anyways.

{
$resolver->setDefault('translation_domain', 'validators');
}

public function buildForm(FormBuilderInterface $builder, array $options): void
{
foreach ($builder->getData()->getCollectedFields() as $identifier => $fieldData) {
/** @var FieldData $fieldData */
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this @var? If yes, please use FQCN in the typehint.

if ($fieldData->fieldDefinition->fieldTypeIdentifier === 'ezemail') {
$emailOptions = $builder->get($identifier)->getOptions();

$customEmailExists = false;
foreach ($emailOptions['constraints'] as $constraint) {
if ($constraint instanceof CustomEmail) {
$customEmailExists = true;

break;
}
}

if (!$customEmailExists) {
$emailOptions['constraints'][] = new CustomEmail();
}

$builder->add($identifier, InformationCollectionFieldType::class, $emailOptions);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

declare(strict_types=1);

namespace Netgen\Bundle\SiteBundle\InfoCollection\Form\Extension;

use Ibexa\Contracts\ContentForms\Data\Content\FieldData;
use Netgen\Bundle\InformationCollectionBundle\Ibexa\ContentForms\InformationCollectionFieldType;
use Netgen\Bundle\InformationCollectionBundle\Ibexa\ContentForms\InformationCollectionType;
use Netgen\Bundle\SiteBundle\InfoCollection\Validator\Constraints\RequiredField;
use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class RequiredFieldFormExtension extends AbstractTypeExtension
{
public static function getExtendedTypes(): iterable
{
return [InformationCollectionType::class];
}

public function configureOptions(OptionsResolver $resolver): void
{
$resolver->setDefaults([
'translation_domain' => 'validators',
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this here, we're not translating anything here, besides validators domain is default anyways.

'attr' => [
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Isn't this something for template to control and not the form extension?

'class' => 'embed-form js-form-embed',
],
]);
}

public function buildForm(FormBuilderInterface $builder, array $options): void
{
foreach ($builder->getData()->getCollectedFields() as $identifier => $fieldData) {
/** @var FieldData $fieldData */
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this @var? If yes, please use FQCN in the typehint.

if ($fieldData->fieldDefinition->isRequired === true) {
$fieldOptions = $builder->get($identifier)->getOptions();

$hasRequiredFieldConstraint = false;

foreach ($fieldOptions['constraints'] as $constraint) {
if ($constraint instanceof RequiredField) {
$hasRequiredFieldConstraint = true;

break;
}
}

if (!$hasRequiredFieldConstraint) {
$fieldOptions['constraints'][] = new RequiredField();
$builder->add($identifier, InformationCollectionFieldType::class, $fieldOptions);
}
}
}
}
}
12 changes: 12 additions & 0 deletions bundle/InfoCollection/Validator/Constraints/CustomEmail.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

declare(strict_types=1);

namespace Netgen\Bundle\SiteBundle\InfoCollection\Validator\Constraints;

use Symfony\Component\Validator\Constraint;

final class CustomEmail extends Constraint
{
public string $message = 'email_is_not_valid';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

declare(strict_types=1);

namespace Netgen\Bundle\SiteBundle\InfoCollection\Validator\Constraints;

use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;
use Symfony\Component\Validator\Exception\UnexpectedTypeException;

use function filter_var;

use const FILTER_VALIDATE_EMAIL;

final class CustomEmailValidator extends ConstraintValidator
{
public function validate(mixed $value, Constraint $constraint): void
{
if (!$constraint instanceof CustomEmail) {
throw new UnexpectedTypeException($constraint, CustomEmail::class);
}

$emailValue = $value->value->email;

if (filter_var($emailValue, FILTER_VALIDATE_EMAIL) === false) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of filter_var, we should use egulias/email-validator which is better and closer to specification.

$this->context->buildViolation($constraint->message)
->atPath($value->fieldDefinition->identifier)
->addViolation();
}
}
}
12 changes: 12 additions & 0 deletions bundle/InfoCollection/Validator/Constraints/RequiredField.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

declare(strict_types=1);

namespace Netgen\Bundle\SiteBundle\InfoCollection\Validator\Constraints;

use Symfony\Component\Validator\Constraint;

final class RequiredField extends Constraint
{
public string $message = 'this_value_should_not_be_blank';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

declare(strict_types=1);

namespace Netgen\Bundle\SiteBundle\InfoCollection\Validator\Constraints;

use Ibexa\Contracts\ContentForms\Data\Content\FieldData;
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;

use function array_filter;
use function property_exists;

final class RequiredFieldValidator extends ConstraintValidator
{
public function validate(mixed $value, Constraint $constraint): void
{
/** @var FieldData $value */
Copy link
Member

Choose a reason for hiding this comment

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

We should check if $value and $constraint are of correct type and just return if not, instead of using type coercion. Something like https://github.com/netgen-layouts/layouts-core/blob/master/lib/Validator/LayoutNameValidator.php#L33-L43

if (
$value->value === null
|| array_filter(
$value->value->attributes(),
static fn ($attr) => $value->value->attribute($attr) === null || $value->value->attribute($attr) === '' || $value->value->attribute($attr) === false,
Copy link
Member

Choose a reason for hiding this comment

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

The closure is missing property and return typehints.

) !== []
) {
if (!property_exists($constraint, 'message')) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to check for existence of message property since we know we're dealing with RequiredField constraint which has the message property?

return;
}
$this->context->buildViolation($constraint->message)
->atPath($value->fieldDefinition->identifier)
->addViolation();
}
}
}
10 changes: 10 additions & 0 deletions bundle/Resources/config/info_collection.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ services:
tags:
- { name: netgen_information_collection.action }

ngsite.info_collection.form.extension.required_field:
class: Netgen\Bundle\SiteBundle\InfoCollection\Form\Extension\RequiredFieldFormExtension
tags:
- { name: form.type_extension, extended_type: Netgen\Bundle\InformationCollectionBundle\Ibexa\ContentForms\InformationCollectionType, priority: -100 }

ngsite.info_collection.form.extension.email_form_extenison:
class: Netgen\Bundle\SiteBundle\InfoCollection\Form\Extension\EmailFormExtension
tags:
- { name: form.type_extension, extended_type: Netgen\Bundle\InformationCollectionBundle\Ibexa\ContentForms\InformationCollectionType, priority: -100 }

ngsite.info_collection.form.extension.honeypot_extension:
class: Netgen\Bundle\SiteBundle\InfoCollection\Form\Extension\HoneypotExtension
tags:
Expand Down
2 changes: 2 additions & 0 deletions bundle/Resources/translations/validators.en.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
this_value_should_not_be_blank: 'This value should not be blank.'
Copy link
Member

Choose a reason for hiding this comment

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

We should add prefixes to these message identifiers:

required_field.this_value_should_not_be_blank and custom_email.email_is_not_valid.

This way we allow adding more messages in the future.

email_is_not_valid: 'Email is not valid.'
Copy link
Member

Choose a reason for hiding this comment

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

Missing EOF at the end of file.