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 extractor for constraints #160

Merged
merged 5 commits into from
Feb 4, 2022
Merged

Conversation

lukepass
Copy link
Contributor

Hello, I created for my project an extractor to get the list of translations from constraints in form (and controller) classes:

class ChangePasswordFormType extends AbstractType
{
    /**
     * @param mixed[]                                           $options
     * @param FormBuilderInterface<FormBuilderInterface|string> $builder
     */
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add('plainPassword', RepeatedType::class, [
                'type' => PasswordType::class,
                'first_options' => [
                    'constraints' => [
                        new NotBlank([
                            'message' => 'Inserisci la nuova password.',
                        ]),
                        new Length([
                            'min' => 6,
                            'minMessage' => 'La nuova password deve essere di almeno {{ limit }} caratteri.',
                            // max length allowed by Symfony for security reasons
                            'max' => 4096,
                        ]),
                    ],
                    'label' => 'Nuova password',
                ],
                'second_options' => [
                    'label' => 'Ripeti password',
                ],
                'invalid_message' => 'Le due password devono corrispondere.',
                // Instead of being set onto the object directly,
                // this is read and encoded in the controller
                'mapped' => false,
            ])
        ;
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults([]);
    }
}

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

What about custom constraints? This implementation won't cover them, right? It only works with Symfony predefined constraints as far as I understand?

Also, any chance you could add some tests to cover the use case with constraints you're going to add?

$isConstraintClass = false;
foreach ($parts as $part) {
if (\in_array($part, self::CONSTRAINT_CLASS_NAMES)) {
$isConstraintClass = true;
Copy link
Member

Choose a reason for hiding this comment

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

If we found the constraint class - should we continue iterating further or stop iteration? I just don't see the value in iterating further

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately if I write Assert\NotBlank instead of just NotBlank the "parts" would contain Assert and NotBlank so I need to iterate all of them.

Copy link
Member

Choose a reason for hiding this comment

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

Can't part of finding an appropiate constraint class get caught by checking against extending Symfony\Component\Validator\Constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I check against extending a Constraint class, that would be useful!

Copy link
Member

Choose a reason for hiding this comment

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

A is_subclass_of function call can get used for that https://3v4l.org/jcgXX

Copy link
Contributor Author

@lukepass lukepass Jul 30, 2021

Choose a reason for hiding this comment

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

Yes, I know that but I can't use is_subclass_of because in the $part I don't have the fully qualified class name but just the "string" of the class name (e.g. "NotBlank"). nikic/PHP-Parser doesn't return the class FQCN.

}

if (!$item->value instanceof Node\Scalar\String_) {
$this->addError($item, 'Constraint message is not a scalar string');
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we need to add an error about this? Could you explain why it's better than just silently ignore non-string values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I like to get errors when translating so that I know that the extractor failed on one part but if you want I can just silently ignore it!

@lukepass
Copy link
Contributor Author

Thank you for this contribution!

What about custom constraints? This implementation won't cover them, right? It only works with Symfony predefined constraints as far as I understand?

Also, any chance you could add some tests to cover the use case with constraints you're going to add?

Yes, unfortunately it will work only for a pre-defined set of constraints. I don't think I can get the class type from the name, right?
I am working on it right now to make a better version (e.g. "minMessage" and other properties, better errors and better code).

@lukepass
Copy link
Contributor Author

I added more features and a small test suite.

@bocharsky-bw
Copy link
Member

Hey @lukepass ! Could you pull the latest changes from master to see if tests pass here?

@bocharsky-bw
Copy link
Member

@lukepass could you take a look at failed PHP-CS-Fixer here?

@lukepass
Copy link
Contributor Author

lukepass commented Feb 3, 2022

Yes, the tests are passing. I already used php-cs-fixer on my code but there was a problem with public const (I am using PHPCSFixer as ruleset).

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Cool! Looks good to me 👍

Thank you for this contribution and sorry for the delay!

@bocharsky-bw bocharsky-bw merged commit 0f1a1a6 into php-translation:master Feb 4, 2022
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.

None yet

3 participants