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

Missing ? in symfony console make:entity? #1513

Open
raffcioo opened this issue Apr 16, 2024 · 9 comments
Open

Missing ? in symfony console make:entity? #1513

raffcioo opened this issue Apr 16, 2024 · 9 comments
Labels
Status: Waiting Feedback Needs feedback from the author Unconfirmed

Comments

@raffcioo
Copy link

Hi! I posted this in main Symfony repo a few days ago and it was closed because it is not a feature, but I was asked to post it here, so here it is. I am not a Symfony expert yet I want to be, really like it and thank you for it, so I may be wrong, just delete this if so and sorry :)

I am using symfony console make:entity, lets assume one field is company string, not nullable.

It generates:

public function setCompany(string $company): static
{
    $this->company = $company;

    return $this;
}

Shouldn't it generate:

public function setCompany(?string $company): static
{
    $this->company = $company;

    return $this;
}

?

When user submits form with empty company I am getting:
HTTP 500. InvalidArgumentException. Expected argument of type "string", "null" given at property path "company".

The thing is that I have validator on this field, in form, so it checks for NotBlank, but it just does not have oportunity to do its job.

If I forget about that validator I am getting:
HTTP 500. An exception occurred while executing a query: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'company' cannot be null
so it is ok in that case.

When I add ? everything works, but I have to do it manually in those setters every time I use that command.

@jrushlow
Copy link
Collaborator

Howdy @raffcioo! I believe make:entity is working as intended. If I create a new Me entity with 2 properties:

  1. name and select "no" or just press enter to "can this field be nullable":
    image

  2. occupation and select "yes" to the nullable question
    image

I end up with (redacted non-relevant parts):

#[ORM\Entity(repositoryClass: MeRepository::class)]
class Me
{
    #[ORM\Column(length: 255)]
    private ?string $name = null;

    #[ORM\Column(length: 255, nullable: true)]
    private ?string $occupation = null;

    public function getName(): ?string { }

    public function setName(string $name): static
    {
        $this->name = $name;

        return $this;
    }

    public function getOccupation(): ?string { }

    public function setOccupation(?string $occupation): static
    {
        $this->occupation = $occupation;

        return $this;
    }
}

As you can see for name the property itself is nullable (this is intended) by the column definition is basically the same as Column(nullable: false). As compared to the occupation property - both the property type && the column definition are nullable.

Because both of the properties are private and the only way to set or change the value of the properties is via the set methods. Adding the argument type ? to a setName(string $name) wouldn't make much sense here - the property cannot be null when persisting the entity. But on the other hand, setOccupation(?string $occupation) is perfectly acceptable because the column definition allows nullable: true.

Just a "pro tip" - for simple forms & entities - adding the assertions to the entity directly can help alot. Symfony's form validator is able to look at the entity and see this assertion when a user submits a form. By doing it like this - the property declaration, column definition, and form validation are all in the same spot

// Entity

    #[Assert\NotBlank()]
    #[ORM\Column(length: 255)]
    private ?string $name = null;

I hope this helps, let me know if you are still having issues are need any further help!

Sidenote: checkout https://symfonycasts.com/ - we have a gazillion tips, tricks, and awesome tutorials over there on how to be a Symfony Jedi Master :D

@jrushlow jrushlow added Status: Waiting Feedback Needs feedback from the author Unconfirmed labels Apr 17, 2024
@raffcioo
Copy link
Author

Hi @jrushlow, thank you for response. It is corrent that make:entity do setters correctly according to what we select, nullable or not. But the question is is it correct in general. To reproduce what I meant...

  1. using make:entity make simple entity with one field string, not nullable
  2. make simple form with NotBlank validator attached to this field
  3. submit this form with empty value

result:
HTTP 500. InvalidArgumentException. Expected argument of type "string", "null" given.

expected result:
HTTP 422, page loads and user can see the form with a message that this field cannot be blank.

fix:
add ? so method can accept null so the validator can work... I just think/thought that this ? is just missing and should be there by default added by make:entity.

@jrushlow
Copy link
Collaborator

Hmm, I'm not seeing a 500 error when submitting a form with missing, blank, or null values when a string is expected. - I'm getting the expected 422 response & associated validation error.

I have a simple app that I use for testing - https://github.com/jrushlow/app-linked/tree/maker/1513 which recreates what is described above. Try creating a PR (based of off the maker/1513 branch) that reproduces the error you're seeing. I think there is something else going one that is causing the 500 error for you. Without seeing the code and the associated stack trace for the error - it's hard to pin point whats going on.

Tests from the reproducer
image

But to answer your question - for setter methods - the only time null should be accepted is if null can be persisted on the entity. (Granted there are edge cases when your entity logic allows for null - but that's outside the scope of this simple example)

private ?string $notNullableInDatabase = null;
public function setNotNullableInDatabase(string $value);

private ?string $canBeNullInDatabase = null;
public function setCanBeNullInDatabase(?string $value);

As an afterthought - double check your migrations are up to do.. bin/console d:m:status -> bin/console m:migration -> bin/console d:m:m

@raffcioo
Copy link
Author

I just made the testing :) created myself what I wrote above, trying to reproduce, in clean environment, and surprise... I got HTTP422. Wow... thats imposible I thought :) So... I kept digging... And... try in Controller firstly load existing entity, then put it into that form, and then show to the user. When user cleans that field and submit that field empty then the HTTP500 occurs.

@raffcioo
Copy link
Author

So updated how to reproduce list looks like this:

  1. create Symfony project
  2. using make:entity make simple entity with one field string, not nullable
  3. make migration, migrate, add one example record to the database
  4. make form with NotBlank validator attached to that field
  5. in Controller load that created test entity from database, put it into that form, and show to the user
  6. clear original form field data and submit it empty

result:
HTTP500, expected HTTP422

@raffcioo
Copy link
Author

It is thrown in: $form->handleRequest($request);

Using symfony console make:entity I have created Test entity which has only one field name (not nullable string) and in database I have one record with id 1 where that name has example value. In this example below I am autowiring but it does not matter. When I get just int id and do ->find(1) myself it is the same result. I also have TestType form where at name field I have NotBlank constraint set and that form has defaults 'data_class' => Test::class.

#[Route('/edit/{id<\d+>}')]
public function edit(Test $test, Request $request): Response
{
    $form = $this->createForm(TestType::class, $test);

    $form->handleRequest($request); // here is that exception thrown on POST request after form submit

    //that is not even executed
    if ($form->isSubmitted()) {
        if ($form->isValid()) {
            //...
  1. I run in web brower /edit/1
  2. I have form with one field with some tekst
  3. I clear that form field
  4. I click submit
  5. I am getting HTTP500 response

@raffcioo
Copy link
Author

This is happening only on edit/update so to notice it we have to send existing (not new) Entity object in form to the user with correct not null data (field string, not nullable) and wait for user to return it empty.

User submits form and sends POST request with that field data (null) on field (string, not nullable). Symfony firstly instantiates that form and tries to populate received POST data to attached underlying Entity object which previously had a different value (taken from db vs taken from request). So it tries to change that and put NULL there. But how Symfony can populate NULL if setter of that object that it tries to use do not accept NULL? It can't and it throws HTTP500 string expected, got null.

When we add ? to the setter then HTTP500 is not happening. Validators can work. And we get correct in that case HTTP422 response.

@raffcioo
Copy link
Author

For me this is clearly a kind of bug because if someone new came to Symfony and do everything correctly and according to official documentation he shouldn't but eventually will get this HTTP500 and probably this will happen on production because he do not catch this earlier. I hope someone can reproduce it and write here because that thing does not want leave me alone :) I may be wrong with details about internal Symfony workings I wrote above because I am not that much into it yet, and in general I am new, sorry for my buggy English. Regards.

@raffcioo
Copy link
Author

This is what I am doing exacly. I can't describe it better:

  1. symfony new test_project --version="7.0.*" --webapp

  2. open this project in editor and run: symfony serve

  3. symfony console make:entity

  • Class name of the entity to create or update: Test
  • Add the ability to broadcast entity updates using Symfony UX Turbo? no
  • New property name: name
  • Field type: string
  • Field length: 255
  • Can this field be null in the database (nullable): no
    (press enter to quit, no more fields, just this one above)
  1. symfony console make:form
  • The name of the form class: TestType
  • The name of Entity or fully qualified model class name that the new form will be bound to (empty for none): Test

It generated form class:

class TestType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add('name')
        ;
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults([
            'data_class' => Test::class,
        ]);
    }
}

I add constraint to name field because it cannot be blank, so:

public function buildForm(FormBuilderInterface $builder, array $options): void
{
    $builder
        ->add('name', TextType::class, [
            'constraints' => [
                new NotBlank(),
            ],
        ])
    ;
}
  1. In .env file I set up correct DATABASE_URL with database name: test.

  2. symfony console doctrine:database:create

  3. symfony console make:migration

  4. symfony console doctrine:migrations:migrate

  5. I insert into test database test table one row where name is raffcioo, so I have one record with id 1.

  6. symfony console make:controller

  • Choose a name for your controller class: MainController

I update created controller, i need only one method to edit my existing in database entity:

namespace App\Controller;

use App\Entity\Test;
use App\Form\TestType;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Attribute\Route;

class MainController extends AbstractController
{
    #[Route('/edit/{id<\d+>}', name: 'app_main_edit', methods: ['GET', 'POST'])]
    public function edit(Test $test, Request $request): Response
    {
        $form = $this->createForm(TestType::class, $test);
        $form->handleRequest($request);

        // here normally are $form->isSubmitted() and $form->isValid() checks
        // but I do not even write them here in this example because that unwanted exception
        // is happening on POST request in that method above and this would not be executed anyway

        return $this->render('main/edit.html.twig', [
            'form' => $form,
        ]);
    }
}

I also update main/edit.html.twig view. It have only this in that template:

{{ form_start(form, {'attr': {'novalidate': 'novalidate'} }) }}
  {{ form_row(form.name) }}
  <button type="submit">Submit</button>
{{ form_end(form) }}

NOW:

  1. I go to web browser and try to edit that existing entity I added before, so i run /edit/1 and I have on screen form with one name field with value raffcioo.

  2. I clear that field, so it is empty.

  3. I hit submit button.

RESULT:

HTTP 500 Internal Server Error
TypeError InvalidArgumentException
Expected argument of type "string", "null" given at property path "name".

in: $form->handleRequest($request);


When I add ? to Test entity setName setter, so I allow null, this above exception do not happen, I have correct behaviour with HTTP422 response with: This value should not be blank.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Waiting Feedback Needs feedback from the author Unconfirmed
Projects
None yet
Development

No branches or pull requests

2 participants