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

[FrameworkBundle] Added `ControllerTrait::isFormValid` #24576

Merged
merged 1 commit into from Jan 2, 2019

Conversation

@lyrixx
Copy link
Member

commented Oct 16, 2017

Q A
Branch? master (4.1)
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#8518
@weaverryan

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

Hmm, I’m not sure. The form code in a controller suffers from 2 issues: it is a bit verbose, but more importantly, the flow confuses beginners (especially since we typically put both the GET and POST in one controller). By making people do ‘if $form->isSubmitted() && $form->isValid()’, it made things more clear, but uglier.

In other words, I’d like to solve this issue, but I don’t think this is it. Perhaps changing the recommendation to split forms into 2 different controllers but adding some shortcuts (if needed) to help with this is a better thing to investigate. Also, see what other libs do.

*
* @final since version 4.1
*/
protected function isFormValid(FormInterface $form, Request $request = null)

This comment has been minimized.

Copy link
@sstok

sstok Nov 11, 2017

Contributor

You can use a return-type as this is PHP 7.1 ❤️ 😄

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

shall we close then?

@weaverryan

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

Unless @lyrixx has some counter argument, +1 for closing

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2018

Actually, I like this PR. It makes usage of form simpler.

And doing 2 controller for handling a form is wrong IMHO because it leads to code duplication.

*
* @final since version 4.1
*/
protected function isFormValid(FormInterface $form, Request $request = null)

This comment has been minimized.

Copy link
@yceruto

yceruto Feb 5, 2018

Contributor

In order to deduce what this internally does, and more accurate with what we have currently ($form->isSubmitted() && $form->isValid()) what about naming it: isSubmittedFormValid()?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

I'm always in favor of simplifying things and making code as concise as possible ... but I don't like this proposal for two reasons:

  • It introduces an alternative way to do something. That requires documenting, teaching and learning the new way. That's why I only like alternative ways when the new way is 10x better than the older one. (See symfony/symfony-docs#8159 for an example of a new feature introduced as an alternative way of doing something ... but we don't even document it because it's not that great and explaining it complicates things).
  • It hides a lot of things. The new way is just ->isFormValid() ... but internally you are processing/handling the form, checking if it was submitted or not, etc.)
@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2018

It introduces an alternative way to do something. That requires documenting, teaching and learning the new way.

It's not an alternative way, it's a shortcut. This is really different. All method in this trait are shortcut. And they are here just to ease our life.

That's why I only like alternative ways when the new way is 10x better than the older one

indeed, it's not 10x better, it's 2.6x better

proof
$before = strlen('$form->handleRequest($request)->isSubmitted() && $form->isValid()');

$after = strlen('$this->formIsValid($form)');

dump($before/$after);

It hides a lot of things.

Indeed. That's not the primary goal, but it's a side effect.
Writing the same line again and again is really boring.
And it's the same for the forward for example

@weaverryan

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

Hmm, I just don’t like it. It’s shorter, but way less clear. From training, I already need to explain how handleRequest() only handles the request in submit. But at least it’s mostly descriptive.

I can’t think of a better alternative. It really does need to be descriptive and short. Maybe:

if ($this->submit($form) && $form->isValid())

... where the submit() method would call $form->handleRequest() and then return $form->isSubmitted() ?

@gharlan

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

if ($this->isFormSubmittedAndValid($form)) { }

?

@weaverryan

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

I still want some line that looks like we’re doing an action (i.e. submitting the form). That looks like we’re simply checking something. That’s why I suggested $this->submit(). It’s still in the if statement, but hopefully looks like we’re doing something, not just checking :)

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2018

There are too many 👎 from the core team. I have to close this PR
But I would be very happy if someone find an easy (short) solution to submit + validate form.

@lyrixx lyrixx closed this Feb 8, 2018

@lyrixx lyrixx deleted the lyrixx:form-is-valid branch Feb 8, 2018

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

@lyrixx Can you reopen so we can reconsider this PR given the comments in #27638?

@lyrixx lyrixx restored the lyrixx:form-is-valid branch Sep 25, 2018

@lyrixx lyrixx reopened this Sep 25, 2018

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2018

@xabbuh Done

@lyrixx lyrixx force-pushed the lyrixx:form-is-valid branch 3 times, most recently from a4762fb to b4e9ffc Sep 25, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Not sure why discussion on #27638 would influence the decision on this PR. I don't have any strong opinion on the matter (even if I prefer explicitness whenever possible). Let's do a vote in the core team and take a decision once and for all:

1/ Merge as is
2/ Acknowledge that we need to find a "shortcurt" but not the one in this PR
3/ Close this PR and the issue

/cc @symfony/deciders

@Tobion

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

3/ due to #27638 (comment)

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

@lyrixx proposal looks interesting.

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Oct 14, 2018

@lyrixx lyrixx force-pushed the lyrixx:form-is-valid branch from b4e9ffc to 22bd531 Nov 14, 2018

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2018

I have rebased and added the check ($form->isSubmitted())

throw new \LogicException('You must pass a request as second argument because the request stack was empty.');
}
if ($form->isSubmitted()) {

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 14, 2018

Contributor

can be the first check

This comment has been minimized.

Copy link
@HeahDude

HeahDude Nov 14, 2018

Member

What about:

if (!$form->isSubmitted()) {
    $form->handleRequest($request);
}

return $form->isSubmitted() && $form->isValid();

?
Throwing the exception here does not make sense to me due to the name of the shortcut, and given that the form would already do it by itself.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 14, 2018

Contributor

then we conditionally ignore $request which could make this shortcut more confusing. IMHO it's not meant to be used once the form is already handled.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Nov 14, 2018

Author Member

@ro0NL I updated the PR ; thanks

This comment has been minimized.

Copy link
@HeahDude

HeahDude Nov 24, 2018

Member

I'm still not happy with this. Either we should rename the method to better explicit that it submits the form somehow, or to change as I suggest. I would be in favor of a better name like:

ControllerTrait:isFormSubmitted(FormInterface $form, bool $andValid = true, Request $request = null): bool

Then it's clear that one can call FormInterface::isSubmitted() or ControllerTrait::isFormSubmitted() to wrap the handleRequest() call once.
And having a parameter for a strict valid check would allow to use the shortcut even if the logic needs two steps in the controller.

This comment has been minimized.

Copy link
@HeahDude

HeahDude Nov 24, 2018

Member

@ro0NL

IMHO it's not meant to be used once the form is already handled.

It may, think about forward call(s).

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 25, 2018

Contributor

really? That means the form instance is passed to the sub-request, to be validated twice.. 🤔

This comment has been minimized.

Copy link
@HeahDude

HeahDude Nov 25, 2018

Member

Actually it can be recreated and rehandled through this short cut yes, otherwise we should not care about having this clause guard in the first place, but people can do weird things with forward call like chaining or multiple calls during the same request.
So given the name of the shortcut, and unless we can't get it more explicit that it mutates the form, we should make it silent here and just return a bool, IMHO that would avoid WTF moments.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 25, 2018

Contributor

otherwise we should not care about having this clause guard in the first place

hm, that is actually required to do so... as the "default crash" might not always happen (depending on the submitted data in the request) and so we should let it return false:

public function submit($submittedData, $clearMissing = true)
{
if ($this->submitted) {
throw new AlreadySubmittedException('A form can only be submitted once');
}

Also i think we should use a different name indeed. What about submitForm(...): bool?

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 25, 2018

Contributor

actually no :) the check is fine. But intend should be clarified by name (submitForm()).

Removing it would cause to return a possible previous state.

@lyrixx lyrixx force-pushed the lyrixx:form-is-valid branch from 22bd531 to e029e62 Nov 14, 2018

@lyrixx lyrixx force-pushed the lyrixx:form-is-valid branch from e029e62 to 58d457a Nov 26, 2018

@lyrixx lyrixx force-pushed the lyrixx:form-is-valid branch from 58d457a to 1a8c844 Nov 26, 2018

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

given this some more thought, i think using the base controller as a one-size-fits-all solution is a dead-end eventually. It does too much IMHO, and only enables multiple ways of doing things.

While valid by design, i hope long-term we'd strive for standalone solutions, e.g. #29574 instead of approaches like this PR or #28875.

my2cnt :)

@fabpot

fabpot approved these changes Jan 2, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Thank you @lyrixx.

@fabpot fabpot merged commit 1a8c844 into symfony:master Jan 2, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

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

feature #24576 [FrameworkBundle] Added `ControllerTrait::isFormValid`…
… (lyrixx)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[FrameworkBundle] Added `ControllerTrait::isFormValid`

| Q             | A
| ------------- | ---
| Branch?       | master (4.1)
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | symfony/symfony-docs#8518

Commits
-------

1a8c844 [FrameworkBundle] Added `ControllerTrait::isFormValid`

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

@goetas

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

I really liked the "old" way of submitting forms. It was making clear that there was involved a $request and a $form. Two different things that should not be mixed.

This feature allows to do something as:

public function myAction() 
{
    $form = // create form
    if ($this->isFormValid($form)) {
      // something
    }
}

The trait hides the $request object making the isFormValid look as magic and because of if, it seems that $form already contains the request data.
The previous approach was forcing users to learn about the $request object, IMO a fundamental piece of the symfony success.

Im not really against having this method in the trait, but in my opinion the $request parameter should be mandatory.

Is there any chance to rever this or at lease make the $request required?

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

I love this new isFormValid() shortcut ... but I think I agree with @goetas and we should probably make $request mandatory (we'll do that in the Symfony Docs at least). The Request object is too important here to ignore it and get it indirectly.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

I also agree we should make the request mandatory. I will open a new PR

symfony-splitter pushed a commit to symfony/framework-bundle 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 | symfony/symfony#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
-------

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

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()

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.