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

[Form] Enable empty root form name #2936

Merged
merged 4 commits into from Jan 7, 2012
Merged

[Form] Enable empty root form name #2936

merged 4 commits into from Jan 7, 2012

Conversation

canni
Copy link
Contributor

@canni canni commented Dec 21, 2011

BC Break: no
Feature addition: yes
Symfony2 tests pass: yes
Fixes the following issues: #2790
Todo: need more testing

This PR enables usage of empty string as a form name (only at root level).

$id = sprintf('%s_%s', $parentId, $name);
$fullName = sprintf('%s[%s]', $parentFullName, $name);
} else {
if ($parentFullName === $name) {
Copy link
Member

Choose a reason for hiding this comment

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

in this place, parentFullName will always be an empty string so it is better to use an empty string in this check

Copy link
Member

Choose a reason for hiding this comment

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

and you should use elseif throwing the exception instead of putting an if inside the else

@canni
Copy link
Contributor Author

canni commented Dec 21, 2011

@stof corrected

$id = sprintf('%s_%s', $parentId, $name);
$fullName = sprintf('%s[%s]', $parentFullName, $name);
} elseif ('' === $name) {
throw new FormException('Form node with empty name can be used only as root form node.');
Copy link
Member

Choose a reason for hiding this comment

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

I think you are missing a case here:
When $parentFullName !== '' and $name === '' it should also throw an Exception, shouldn't it?

I would rewrite this to

    if ($view->hasParent()) {
        if ('' === $name) {
            throw new FormException('Form node with empty name can be used only as root form node.');
        } elseif ('' !== $parentFullName = $view->getParent()->get('full_name')) {
            $id = sprintf('%s_%s', $view->getParent()->get('id'), $name);
            $fullName = sprintf('%s[%s]', $parentFullName, $name);
        } else {
            $id = $name;
            $fullName = $name;
        }
    } else {

@canni
Copy link
Contributor Author

canni commented Jan 3, 2012

@Tobion corrected, thx

$fullName = sprintf('%s[%s]', $parentFullName, $name);
if ('' === $name) {
throw new FormException('Form node with empty name can be used only as root form node.');
} elseif ('' !== ($parentFullName = $view->getParent()->get('full_name'))) {
Copy link
Member

Choose a reason for hiding this comment

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

you should use if instead of elseif as the previous block leaves the execution of the method

@fabpot
Copy link
Member

fabpot commented Jan 6, 2012

@canni: Can you add some unit tests for this new feature? Thanks.

@canni
Copy link
Contributor Author

canni commented Jan 6, 2012

@fabpot Will do :)

BC Break: no
Feature addition: yes
Symfony2 tests pass: yes
Fixes the following issues: #2790
Todo: need more testing

This PR enables usage of empty string as a form name (only at root level).
@canni
Copy link
Contributor Author

canni commented Jan 7, 2012

@fabpot done.

fabpot added a commit that referenced this pull request Jan 7, 2012
Commits
-------

3702541 Add tests
4f38b14 Remove `elseif`
a5efe6a Add info to CHANGELOG-2.1
3d20e39 [Form] Enable empty root form name

Discussion
----------

[Form] Enable empty root form name

BC Break: no
Feature addition: yes
Symfony2 tests pass: yes
Fixes the following issues: #2790
Todo: need more testing

This PR enables usage of empty string as a form name (only at root level).

---------------------------------------------------------------------------

by canni at 2011/12/21 09:44:51 -0800

@stof corrected

---------------------------------------------------------------------------

by canni at 2012/01/03 01:23:37 -0800

@Tobion corrected, thx

---------------------------------------------------------------------------

by fabpot at 2012/01/05 19:22:25 -0800

@canni: Can you add some unit tests for this new feature? Thanks.

---------------------------------------------------------------------------

by canni at 2012/01/06 03:09:39 -0800

@fabpot Will do :)

---------------------------------------------------------------------------

by canni at 2012/01/07 07:36:10 -0800

@fabpot done.
@fabpot fabpot merged commit 3702541 into symfony:master Jan 7, 2012
$id = $name;
// If this form node have empty name, set id to `form`
// to avoid rendering `id=""` in html structure
$id = $name ?: 'form';
Copy link
Member

Choose a reason for hiding this comment

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

What if you have multiple forms with no name? Then multiple id="form" is also invalid.
So instead it should not render an id attribute at all when the name is empty.
Anyway where does the id attribute for the root form get rendered? I think this only applies to the individual "widgets" inside the root form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same result you get, when you use two forms generated with 2 form builders without submitting its names. But anyway this feature has only a small amount of use cases, and I don't think anyone will render two forms with empty names in single page.

Użytkownik Tobias Schultze reply@reply.github.com napisał:

     } else {
  •        $id = $name;
    
  •        // If this form node have empty name, set id to `form`
    
  •        // to avoid rendering `id=""` in html structure
    
  •        $id = $name ?: 'form';
    

What if you have multiple forms with no name? Then multiple id="form" is also invalid.
So instead it should not render an id attribute at all when the name is empty.
Anyway where does the id attribute for the root form get rendered? I think this only applies to the individual "widgets" inside the root form.


Reply to this email directly or view it on GitHub:
https://github.com/symfony/symfony/pull/2936/files#r334868

Copy link

Choose a reason for hiding this comment

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

i'd use this on all my forms and i do have multiple forms on a page.

i see two options:

  1. still allow the form name to be set but a way to specify the root name?
  2. don't render the id attribute

I'd prefer to not render the id attribute and leave it up to the developer to do so. SF2 is getting too "magical" :P

Copy link
Member

Choose a reason for hiding this comment

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

To 1) I dont understand.
To 2) thats what I proposed. But I'm still not sure in which case and for which field the id=""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jstout24 @Tobion this feature is a small hack, to provide support for rare use cases, when you're forced to have that form (ie. PayPal integration etc.) this should not be used as every-day practice?

@jstout24 what is yours point to have multiple forms with empty name on single page, and why to have ALL of them with empty name? IMO that's pointless...

@Tobion not rendering id="" at all is possible, I was thinking about it but I decided not to...

I think one of you should open a ticket for it, so community can decide :) @fabpot @bschussek what do you think?

Copy link

Choose a reason for hiding this comment

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

@canni ie: my current project has many forms that post via GET (reports, preview pages, etc).. So if I run a report and want to send the link to someone, it looks like:

http://example.com/reports/overall?overall_report[start]=0000-00-00&overall_report[end]=0000-00-00&overal_report[offers][0]=1&overal_report[offers][1]=2

when I'd rather have it look like:

http://example.com/reports/overall?start=0000-00-00&end=0000-00-00&offers[0]=1&offers[1]=2

The above example can go for way more things other than just reports. We also have forms that need to be pre-popped and it gets kind of annoying to explain to our partners that they have to do form_name[first_name] in their forms when they are always used to just passing in ?first_name=... which a lot of our partners systems don't even allow for this functionality.... so we're then forced to create an entire new version to convert our partner's parameters to the way our form does it of our system and deploy it amongst all of our application servers (heavy process for a simple change). I mean I can probably make what we're doing better, but this is everyday use on everyday projects for us.

As far as having multiple forms... you never submit a form more than once. I've always been used to having a hidden field with a "form name" and checking that on POST or GET.

@frequenc1
Copy link

trying to figure out how to use this switch.

@stof
Copy link
Member

stof commented Jun 20, 2012

@frequenc1 the form factory has a createNamedBuilder method (and the corresponding createNamed method, which allow passing a name instead as using the name of the root type as root name of the form.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants