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

[FrameworkBundle] Remove ControllerTrait::isFormValid() #29813

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
@lyrixx
Copy link
Member

lyrixx commented Jan 8, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #24576 (comment)
License MIT
Doc PR

edit: During the first draft I made, I did not use the request stack. I finally used it to mimic other shortcut! It was a bad idea. Now there is less code, it's simpler. Love it more

@xabbuh

xabbuh approved these changes Jan 8, 2019

@stof

stof approved these changes Jan 8, 2019

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 8, 2019

@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!

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 8, 2019

@javiereguiluz 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.

So let's keep thing simple here. I really prefer isFormValid(). There are no need to explain about isSubmitted()

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 8, 2019

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:

public function new(Request $request)
{
    $task = new Task();
    $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:

public function new(Request $request)
{
    $task = new Task();
    $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!

@mbabker

This comment has been minimized.

Copy link
Contributor

mbabker commented Jan 8, 2019

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

@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Jan 10, 2019

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:

public function new(Request $request)
{
    $task = new Task();
    $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 with is 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) - or if ($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 added isSubmitted(), there WAS still handleRequest(). 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.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 10, 2019

@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.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 10, 2019

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

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 10, 2019

@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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 10, 2019

I think we should remove the method personally.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 10, 2019

I would vote to revert as well.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 10, 2019

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 ?

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 10, 2019

@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".

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Jan 10, 2019

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

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 10, 2019

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 typing if ($form->handleRequest($request)->isSubmitted()) && $form->isValid()) { 😂
So Let's revert it :)

@lyrixx lyrixx force-pushed the lyrixx:form-is-valid branch from 218e6f7 to fd2985c Jan 10, 2019

@xabbuh
Copy link
Member

xabbuh left a comment

@lyrixx Can you change the PR title to better match what we now have here? :)

@lyrixx lyrixx changed the title [FrameworkBundle] Make the request mandatory in ControllerTrait::isFormValid() [FrameworkBundle] Remove ControllerTrait::isFormValid() Jan 11, 2019

@zanbaldwin

This comment has been minimized.

Copy link
Contributor

zanbaldwin commented Jan 11, 2019

Changing array() 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 in symfony/flex and not the components 🤷‍♀️

@ostrolucky

This comment has been minimized.

Copy link
Contributor

ostrolucky commented Jan 11, 2019

All the concerns raised could be solved by using @javiereguiluz's suggestion isFormSubmittedAndValid. 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 for isSubmitted && isValid

@gharlan

This comment has been minimized.

Copy link
Contributor

gharlan commented Jan 11, 2019

Maybe just add a isSubmittedAndValid() shortcut to form instead of ControllerTrait, and without implicit handleRequest?

2019-01-11 14 59 11

vs.

2019-01-11 15 01 43

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

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Jan 11, 2019

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

@ostrolucky

This comment has been minimized.

Copy link
Contributor

ostrolucky commented Jan 12, 2019

No exception, it's not implied anywhere

@zanbaldwin

This comment has been minimized.

Copy link
Contributor

zanbaldwin commented Jan 14, 2019

Just as a minor public service announcement, I just found out it was quietly decided 6 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

@fabpot fabpot force-pushed the lyrixx:form-is-valid branch from 3be64ae to 2be1987 Jan 14, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 14, 2019

Thank you @lyrixx.

@fabpot fabpot merged commit 2be1987 into symfony:master Jan 14, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 14, 2019

feature #29813 [FrameworkBundle] Remove ControllerTrait::isFormValid(…
…) (lyrixx)

This PR was squashed before being merged into the 4.3-dev branch (closes #29813).

Discussion
----------

[FrameworkBundle] Remove ControllerTrait::isFormValid()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24576 (comment)
| License       | MIT
| Doc PR        |

**edit**: During the first draft I made, I did not use the request stack. I finally used it to mimic other shortcut! It was a bad idea. Now there is less code, it's simpler. Love it more

Commits
-------

2be1987 [FrameworkBundle] Remove ControllerTrait::isFormValid()

@lyrixx lyrixx deleted the lyrixx:form-is-valid branch Jan 14, 2019

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