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

Symfony validator no emphasize on public properties inside Constraint class #20406

Open
skyferix opened this issue Nov 23, 2024 · 5 comments
Open
Labels
hasPR A Pull Request has already been submitted for this issue. Validator

Comments

@skyferix
Copy link

skyferix commented Nov 23, 2024

I have written custom validator (SequenceValidator). Validator fails unexpectedly when we call getRegex() method on Sequence constraint object I have written example below:

#[\Attribute(\Attribute::TARGET_PROPERTY)]
class Sequence extends Constraint
{
    public function __construct(private readonly string $regex, mixed $options = null, ?array $groups = null, mixed $payload = null)
    {
        parent::__construct($options, $groups, $payload);
    }

    public function getRegex(): string
    {
        return $this->regex;
    }
}

The strange thing is that it will populate properties once on first run and not on other rounds (For example of HTTP request perspective).
It happens only in when environment is set to prod.

The only indication that only public variables are properly used for validator constraint is shown in comment when you use Symfony\Bundle\MakerBundle\MakerBundle and make validator in this case bin/console make:validator SequenceValidator

#[\Attribute(\Attribute::TARGET_PROPERTY)]
class Sequence extends Constraint
{
    /*
     * Any public properties become valid options for the annotation.
     * Then, use these in your validator class.
     */
    public string $message = 'The value "{{ value }}" is not valid.';
}

I would like ether add documentation in symfony documentation directly as warning or add it to App\Validator\Contraint class itself to make sure that developers are using the functionality correctly.

@skyferix skyferix changed the title Symfony validator no empasize on public properties inside Constraint class Symfony validator no emphasize on public properties inside Constraint class Nov 23, 2024
@xabbuh
Copy link
Member

xabbuh commented Nov 27, 2024

I am not sure I understand the issue here. Can you explain it with more details or create a small example application that allows to reproduce?

@skyferix
Copy link
Author

Sure, will provide example later this week

@skyferix
Copy link
Author

skyferix commented Dec 1, 2024

Sorry for confusion earlier.
So again, the issue I have is that validation constraints with private fields are not properly populated when APP_DEBUG is set to false.

Reproduced in symfony 5.4.* and 7.1.*

How to reproduce:

  1. APP_DEBUG=false => .env.local
  2. bin/console cache:clear
  3. bin/console app:test-validator
  4. bin/console app:test-validator (will fail on second time). With error "Typed property App\Command\Regex::$regex must not be accessed before initialization"

Expected result

If it is expected behaviour and properties on constraint need to be public it should be mentioned in documentation or on Constraint class itself. If not then we need to forward it to validator package maintainers :-)

Setup:

symfony new test-validator --version="7.1.*
composer require symfony/validator

Code:

<?php

namespace App\Command;

use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;
use Symfony\Component\Validator\Validator\ValidatorInterface;

#[AsCommand(
    name: 'app:test-validator',
    description: 'Add a short description for your command',
)]
class TestValidatorCommand extends Command
{
    public function __construct(private readonly ValidatorInterface $validator)
    {
        parent::__construct();
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $dto = new DTO();
        $dto->setIp('127.0.0.1');

        $this->validator->validate($dto);

        return Command::SUCCESS;
    }
}

class DTO
{
    #[Regex(Regex::IP_ADDRESS)]
    private string $ip;

    public function getIp(): string
    {
        return $this->ip;
    }

    public function setIp(string $ip): void
    {
        $this->ip = $ip;
    }
}

// This is just an example of custom constraint
#[\Attribute(\Attribute::TARGET_PROPERTY)]
class Regex extends Constraint
{
    public const IP_ADDRESS = "/^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$/";

    public const REGEX_MATCHES = 1;

    public function __construct(
        private readonly string $regex,
        mixed                   $options = null,
        ?array                  $groups = null,
        mixed                   $payload = null
    )
    {
        parent::__construct($options, $groups, $payload);
    }

    public function getRegex(): string
    {
        return $this->regex;
    }
}

class RegexValidator extends ConstraintValidator
{
    /**
     * @param Regex $constraint
     */
    public function validate(mixed $value, Constraint $constraint): void
    {
        if (null === $value || '' === $value) {
            return;
        }
        if ($this->regexMatches($constraint, $value)) {
            return;
        }

        $this->context->buildViolation('Invalid value');
    }

    private function regexMatches(Regex $constraint, $value): bool
    {
        // error happens when getRegex is called
        return $constraint::REGEX_MATCHES !== preg_match($constraint->getRegex(), $value);
    }
}

@xabbuh
Copy link
Member

xabbuh commented Dec 2, 2024

Thank you for the details. I understand the issue now. What happens is that constraints are cached. For that the base Constraint class implements the __sleep() method to indicate what data to cache. This implementation uses PHP's get_object_vars() function that doesn't cover private properties of the child classes.

We should probably add a warning that you need to overwrite __sleep() if your constraint's properties are private.

@alamirault
Copy link
Contributor

I confirm issue, and confirm your proposal works

I created PR #20406

@xabbuh xabbuh added the hasPR A Pull Request has already been submitted for this issue. label Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hasPR A Pull Request has already been submitted for this issue. Validator
Projects
None yet
Development

No branches or pull requests

3 participants