@weaverryan I'd like to read your opinion about this. In the past you insisted a lot to add the$form->isSubmitted() && ... to all the doc examples to be more explicit. I think that's the right decision because code is easier to understand (otherwise newcomers can get lost:who/when/how was the form submitted !?!?)

So, my question: are you happy with the proposed shortcut name?

if ($this->isFormValid($form,$request)) {... }

Or would you prefer to keep being explicit about the form submission?

if ($this->isFormSubmittedAndValid($form,$request)) {... }if ($this->formIsSubmittedAndValid($form,$request)) {... }...

Thanks!

@javiereguiluz we addedisSubmitted() everywhere in the doc to fix an inconsistency in the Form Component. This is just a hack. Before the inconsistency was raised, almost nobody usedisSubmitted() and everything were fine.

So let's keep thing simple here. I really preferisFormValid(). There are no need to explain aboutisSubmitted()

I've seen lots of newcomers have problems understand where or who submits the form. With the current recommended way of working, you can easily see the "is submitted?" check:

publicfunctionnew(Request$request){$task =newTask();$form =$this->createForm(TaskType::class,$task);$form->handleRequest($request);if ($form->isSubmitted() &&$form->isValid()) {// ...return$this->redirectToRoute('task_success');    }return$this->render('task/new.html.twig', ['form' =>$form->createView(),    ]);}

If you remove the optional$form->isSubmitted() ... the code is harder to understand but newcomers think:"OK, I guess the form submission will take place in this weird $form->handleRequest($request) call which I don't understand (yet)."

My fear is that the new shortcut removes any clues about the form being submitted or processed or handled:

publicfunctionnew(Request$request){$task =newTask();$form =$this->createForm(TaskType::class,$task);if ($this->isFormValid($form,$request)) {// ...return$this->redirectToRoute('task_success');    }return$this->render('task/new.html.twig', ['form' =>$form->createView(),    ]);}

But my fears could be exaggerated. That's why I want to know Ryan's opinion about this. Thanks!

If the form being submitted is a condition of it being valid, then I would suggestisFormValid is perfectly fine as far as naming goes.

Sorry@lyrixx, I don't really like this new shortcut, though it looks like Fab does like it, so it's certainly subjective. As Javier mentioned, with:

publicfunctionnew(Request$request){$task =newTask();$form =$this->createForm(TaskType::class,$task);if ($this->isFormValid($form,$request)) {// ...return$this->redirectToRoute('task_success');    }return$this->render('task/new.html.twig', ['form' =>$form->createView(),    ]);}

The flow is very non-obvious. It's very strange to have a method starting withis that performs an action (an important action). And I can't really think of anything I love :/. But I'm also not sure the current/old way is a huge problem.

If anything, I tend to agree with Javier's suggestions:#29813 (comment) - orif ($this->submitForm($form, $request) && $form->isValid())

lyrixx we added isSubmitted() everywhere in the doc to fix an inconsistency in the Form Component. This is just a hack. Before the inconsistency was raised, almost nobody used isSubmitted() and everything were fine.

Yes, but before we addedisSubmitted(), there WAS stillhandleRequest(). So you could guess (even though the name isn't great) that this is the line that's probably submitting things. Removing any line that suggests the form being submitted is where things get slippery.

@lyrixx With this PR, I think the shortcut has less "value". It was already quite controversial, but reading@weaverryan and@javiereguiluz comments, I think I tend to agree with them now.

What should I do? I'm a bit confused here :|

@lyrixx I still like this shortcut ... but I think we need to tweak it a bit to make it better. I don't have a perfect solution for this, though.

As mentioned, controllers handling forms have two very different execution branches ... and this shortcut hides that too much at the moment.

I think we should remove the method personally.

I would vote to revert as well.

As mentioned, controllers handling forms have two very different execution branches ... and this shortcut hides that too much at the moment.

Are you talking about submitted vs valid ?

@lyrixx no, I was talking about creating/rendering the form vs processing the form. That's why I need the "isSubmitted()" thing ... to say,"yes! now it's when we are processing the form instead of rendering it".

Too much important things are hidden by this shortcut, the name does not reflect what it does. I don't expect aisFormValid() method to throw if the form is submitted, nor I expect it to process submission by itself. +1 for reverting.

Actually, I just wanted to replace

if ($form->handleRequest($request)->isSubmitted()) &&$form->isValid()) {# code...}

if ($this->isFormValid($form) {# code...}

I guess I have used the following code only once

if ($form->handleRequest($request)->isSubmitted()) {#code}

I think I spend more time to make theses PR merged than typingif ($form->handleRequest($request)->isSubmitted()) && $form->isValid()) { 😂
So Let's revert it :)

Changingarray() syntax to[] is probably outside the scope of this pull request.

Also, can someone from the core team clarify which syntax Symfony components now use?
Last I heard the only place the new syntax was being used was insymfony/flex and not the components 🤷‍♀️

All the concerns raised could be solved by using@javiereguiluz's suggestionisFormSubmittedAndValid. Not sure why revert was chosen instead of simple rename.

edit: to clarify, I'm not implying this should submit the form. This is only shortcut forisSubmitted && isValid

Maybe just add aisSubmittedAndValid() shortcut to form instead of ControllerTrait, and without implicithandleRequest?

vs.

I need ~5.8s to type this instead of ~9.3s (with autocompletion in phpstorm). Don't know, if it is worth it. ;)

And have anisSubmitted...() throwing an exception if the form is not submitted? Sounds plain wrong to me.

No exception, it's not implied anywhere

Just as a minor public service announcement, I just found out it was quietly decided6 days ago that the main Symfony codebase now uses the short array syntax.
Therefore the changes that@lyrixx has included in this PR are the correct way of doing it.
/cc@sstok

Thank you@lyrixx.