Skip to content

[Form] String is the preferred value type for TextType #41514

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

Closed

Conversation

peter-gribanov
Copy link
Contributor

@peter-gribanov peter-gribanov commented Jun 2, 2021

Q A
Branch? 7.0
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #5906
License MIT
Doc PR symfony/symfony-docs#15404

I want to return to an old and very painful topic #5906. I wanted to revisit this topic before the release 4.0 and 5.0, but didn't find the time for this. Now, when everyone already has PHP 7 or 8, and the strict_types=1 is the defect standard, it is probably time to abandon the hook when the text may not be a string.

We've been using hooks for many years, even before #18357. We want string values to always be strings. Under strict typing conditions, i consider using NULL as a value valid only in cases where the expected value is not a scalar type and cannot be correctly transformed from the submitted data. In such cases, we use custom transformers.

Expected implementation

A very crude example.

use Symfony\Component\Validator\Constraints as Assert;

final class RenameAuthor
{
    /**
     * @Assert\NotBlank
     * @Assert\Length(max=128)
     */
    public string $firstname = '';

    /**
     * @Assert\Length(max=128)
     */
    public string $lastname = '';

    /**
     * @Assert\Length(max=128)
     */
    public string $patronymic = '';
}
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class RenameAuthorFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add('firstname', TextType::class)
            ->add('lastname', TextType::class, [
                'required' => false,
            ])
            ->add('patronymic', TextType::class, [
                'required' => false,
            ])
        ;
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefault('data_class', RenameAuthor::class);
    }
}
class Questionnaire
{
    // ...

    public function renameAuthor(string $new_firstname, string $new_lastname, string $new_patronymic): void
    {
        $this->author_name = new AuthorName($new_firstname, $new_lastname, $new_patronymic);
    }
$command = new RenameAuthor();

$form = $this->createForm(RenameAuthorFormType::class, $command)->handleRequest($request);

if ($form->isSubmitted() && $form->isValid()) {
    $questionnaire->renameAuthor($command->firstname, $command->lastname, $command->patronymic);

    // ...
}

Actual implementation

Without hooks and specifying option empty_data.

use Symfony\Component\Validator\Constraints as Assert;

final class RenameAuthor
{
    /**
     * @Assert\NotBlank
     * @Assert\Length(max=128)
     */
-    public string $firstname = '';
+    private string $firstname = '';

    /**
     * @Assert\Length(max=128)
     */
-    public string $lastname = '';
+    private string $lastname = '';

    /**
     * @Assert\Length(max=128)
     */
-    public string $patronymic = '';
+    private string $patronymic = '';
+
+    public function getFirstname(): string
+    {
+        return $this->firstname;
+    }
+
+    public function setFirstname(?string $firstname): void
+    {
+        $this->firstname = $firstname ?? '';
+    }
+
+    public function getLastname(): string
+    {
+        return $this->lastname;
+    }
+
+    public function setLastname(?string $lastname): void
+    {
+        $this->lastname = $lastname ?? '';
+    }
+
+    public function getPatronymic(): string
+    {
+        return $this->patronymic;
+    }
+
+    public function setPatronymic(?string $patronymic): void
+    {
+        $this->patronymic = $patronymic ?? '';
+    }
}
$command = new RenameAuthor();

$form = $this->createForm(RenameAuthorFormType::class, $command)->handleRequest($request);

if ($form->isSubmitted() && $form->isValid()) {
-    $questionnaire->renameAuthor($command->firstname, $command->lastname, $command->patronymic);
+    $questionnaire->renameAuthor($command->getFirstname(), $command->getLastname(), $command->getPatronymic());

    // ...
}

Implementation with empty_data option

It seems that specifying one option is not a problem. But you should always remember that the text may not be a string, and to typecast to a string, you need to specify option empty_data in each field, each form. There may be hundreds and thousands of such places per project, and the probability of errors increases with the growth of the project.

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class RenameAuthorFormType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
-            ->add('firstname', TextType::class)
+            ->add('firstname', TextType::class, [
+                'empty_data' => '',
+            ])
            ->add('lastname', TextType::class, [
+                'empty_data' => '',
                'required' => false,
            ])
            ->add('patronymic', TextType::class, [
+                'empty_data' => '',
                'required' => false,
            ])
        ;
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefault('data_class', RenameAuthor::class);
    }
}

@stof
Copy link
Member

stof commented Jun 2, 2021

this change is a BC break for projects that rely on getting null. Doing that in 6.0 without an upgrade path is a no-go

@peter-gribanov peter-gribanov force-pushed the form_text_type_stringable branch 2 times, most recently from 4217bfd to 66fb803 Compare June 2, 2021 14:40
@peter-gribanov
Copy link
Contributor Author

@stof quite right. That is why i propose to introduce this in the major version, in which we can break BC. What do you think about this #41516 ?

@peter-gribanov
Copy link
Contributor Author

I would like to get some response. Is it a bad idea to give up the need to specify the empty_data option every time you use TextType and types inherited from TextType?

I agree that an unsubmitted value is not equivalent to empty value and it is correct to provide the user of Form component with the ability to handle these cases differently, but do users need to handle them separately? Is it possible to give some example in which the use of NULL in TextType is justified when using strict types?

@fabpot fabpot modified the milestones: 6.0, 6.1 Nov 21, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@nicolas-grekas
Copy link
Member

Changing the behavior in the next major is possible only if there is a deprecation in a previous minor.
In this case, I'm not sure it's worth it.
I'm closing here because it didn't get any traction apparently.
Thanks for submitting.

@peter-gribanov
Copy link
Contributor Author

Changing the behavior in the next major is possible only if there is a deprecation in a previous minor.

@nicolas-grekas see #41516

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants