Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FrameworkBundle] AddedControllerTrait::isFormValid#24576

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromlyrixx:form-is-valid
Jan 2, 2019

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedOct 16, 2017
edited
Loading

QA
Branch?master (4.1)
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRsymfony/symfony-docs#8518

welcoMattic, yceruto, juuuuuu, lyrixx, apfelbox, gharlan, and soprun reacted with thumbs up emojijvasseur and ro0NL reacted with confused emoji
@weaverryan
Copy link
Member

Hmm, I’m not sure. The form code in a controller suffers from 2 issues: itis 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.

jvasseur reacted with thumbs up emoji

* @return bool
*
* @final since version 4.1
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@nicolas-grekas
Copy link
Member

shall we close then?

@weaverryan
Copy link
Member

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

@lyrixx
Copy link
MemberAuthor

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.

yceruto reacted with thumbs up emoji

*
* @final since version 4.1
*/
protectedfunctionisFormValid(FormInterface$form,Request$request =null)
Copy link
Member

@ycerutoycerutoFeb 5, 2018
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
Copy link
Member

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. (SeeAllow imports in string format for YAML 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
Copy link
MemberAuthor

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 theforward for example

yceruto and ismail1432 reacted with thumbs up emoji

@weaverryan
Copy link
Member

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
Copy link
Contributor

gharlan commentedFeb 7, 2018
edited
Loading

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

?

sstok reacted with laugh emoji

@weaverryan
Copy link
Member

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 stillin the if statement, but hopefully looks like we’redoing something, not just checking :)

yceruto and gharlan reacted with thumbs up emoji

@lyrixx
Copy link
MemberAuthor

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.

@lyrixxlyrixx closed thisFeb 8, 2018
@lyrixxlyrixx deleted the form-is-valid branchFebruary 8, 2018 10:03
@xabbuh
Copy link
Member

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

lyrixx reacted with thumbs up emoji

@lyrixxlyrixx restored the form-is-valid branchSeptember 25, 2018 11:52
@lyrixxlyrixx reopened thisSep 25, 2018
@lyrixx
Copy link
MemberAuthor

@xabbuh Done ✅

@lyrixxlyrixxforce-pushed theform-is-valid branch 3 times, most recently froma4762fb tob4e9ffcCompareSeptember 25, 2018 12:36
@fabpot
Copy link
Member

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
Copy link
Contributor

Tobion commentedOct 10, 2018
edited
Loading

3/ due to#27638 (comment)

@lyrixx
Copy link
MemberAuthor

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

thrownew \LogicException('You must pass a request as second argument because the request stack was empty.');
}

if ($form->isSubmitted()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

can be the first check

lyrixx reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@ro0NL I updated the PR ; thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 callFormInterface::isSubmitted() orControllerTrait::isFormSubmitted() to wrap thehandleRequest() 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@ro0NL

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

It may, think aboutforward call(s).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

ro0NL reacted with thumbs up emoji
Copy link
Contributor

@ro0NLro0NLNov 25, 2018
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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:

publicfunctionsubmit($submittedData,$clearMissing =true)
{
if ($this->submitted) {
thrownewAlreadySubmittedException('A form can only be submitted once');
}

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Removing it would cause to return a possible previous state.

@ro0NL
Copy link
Contributor

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
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commit1a8c844 intosymfony:masterJan 2, 2019
fabpot added a commit that referenced this pull requestJan 2, 2019
… (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#8518Commits-------1a8c844 [FrameworkBundle] Added `ControllerTrait::isFormValid`
@lyrixxlyrixx deleted the form-is-valid branchJanuary 2, 2019 14:00
@goetas
Copy link
Contributor

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:

publicfunctionmyAction() {$form =// create formif ($this->isFormValid($form)) {// something    }}

The trait hides the$request object making theisFormValid 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
Copy link
Member

I love this newisFormValid() 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.

goetas, florentdestremau, devozanges, and zmitic reacted with thumbs up emoji

@lyrixx
Copy link
MemberAuthor

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

goetas reacted with hooray emoji

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestJan 14, 2019
…) (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 moreCommits-------2be1987ad1 [FrameworkBundle] Remove ControllerTrait::isFormValid()
fabpot added a commit that referenced this pull requestJan 14, 2019
…) (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 moreCommits-------2be1987 [FrameworkBundle] Remove ControllerTrait::isFormValid()
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ycerutoycerutoyceruto left review comments

@fabpotfabpotfabpot approved these changes

+3 more reviewers

@sstoksstoksstok left review comments

@ro0NLro0NLro0NL left review comments

@HeahDudeHeahDudeHeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

15 participants

@lyrixx@weaverryan@nicolas-grekas@javiereguiluz@gharlan@xabbuh@fabpot@Tobion@chalasr@ro0NL@goetas@sstok@yceruto@HeahDude@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp