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.

Sign up for GitHub

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

Jump to bottom

[Form] Enable empty root form name #2936

Merged
merged 4 commits into from
Jan 7, 2012
Merged

[Form] Enable empty root form name #2936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d20e39
[Form] Enable empty root form name
canni Dec 21, 2011
a5efe6a
Add info to CHANGELOG-2.1
canni Jan 3, 2012
4f38b14
Remove `elseif`
canni Jan 3, 2012
3702541
Add tests
canni Jan 7, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG-2.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
* made the translation catalogue configurable via the "translation_domain" option
* added Form::getErrorsAsString() to help debugging forms
* allowed setting different options for RepeatedType fields (like the label)
* added support for empty form name at root level, this enables rendering forms
without form name prefix in field names

### HttpFoundation

Expand Down
19 changes: 14 additions & 5 deletions src/Symfony/Component/Form/Extension/Core/Type/FieldType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,21 @@ public function buildView(FormView $view, FormInterface $form)
$name = $form->getName();

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

if ('' !== ($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 {
$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.

$fullName = $name;
}

Expand Down
21 changes: 16 additions & 5 deletions src/Symfony/Component/Form/Form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,10 @@ public function isReadOnly()
*/
public function setParent(FormInterface $parent = null)
{
if ('' === $this->getName()) {
throw new FormException('Form with empty name can not have parent form.');
}

$this->parent = $parent;

return $this;
Expand Down Expand Up @@ -577,13 +581,20 @@ public function bindRequest(Request $request)
switch ($request->getMethod()) {
case 'POST':
case 'PUT':
$data = array_replace_recursive(
$request->request->get($this->getName(), array()),
$request->files->get($this->getName(), array())
);
if ('' === $this->getName()) {
$data = array_replace_recursive(
$request->request->all(),
$request->files->all()
);
} else {
$data = array_replace_recursive(
$request->request->get($this->getName(), array()),
$request->files->get($this->getName(), array())
);
}
break;
case 'GET':
$data = $request->query->get($this->getName(), array());
$data = '' === $this->getName() ? $request->query->all() : $request->query->get($this->getName(), array());
break;
default:
throw new FormException(sprintf('The request method "%s" is not supported', $request->getMethod()));
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Form/FormFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ private function loadTypeExtensions(FormTypeInterface $type)

private function validateFormTypeName(FormTypeInterface $type)
{
if (!preg_match('/^[a-z0-9_]+$/i', $type->getName())) {
if (!preg_match('/^[a-z0-9_]*$/i', $type->getName())) {
throw new FormException(sprintf('The "%s" form type name ("%s") is not valid. Names must only contain letters, numbers, and "_".', get_class($type), $type->getName()));
}
}
Expand Down
15 changes: 15 additions & 0 deletions tests/Symfony/Tests/Component/Form/AbstractLayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1790,4 +1790,19 @@ public function testCollectionPrototype()
'
);
}

public function testEmptyRootFormName()
{
$form = $this->factory->createNamedBuilder('form', '', '', array(
'property_path' => 'name',
))
->add('child', 'text')
->getForm();

$this->assertMatchesXpath($this->renderWidget($form->createView()),
'//input[@type="hidden"][@id="_token"][@name="_token"]
|
//input[@type="text"][@id="child"][@name="child"]'
, 2);
}
}
6 changes: 6 additions & 0 deletions tests/Symfony/Tests/Component/Form/Extension/Core/Type/FieldTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,4 +254,10 @@ public function testAttributesException()
$form = $this->factory->create('field', null, array('attr' => ''));
}

public function testNameCanBeEmptyString()
{
$form = $this->factory->createNamed('field', '');

$this->assertEquals('', $form->getName());
}
}
84 changes: 84 additions & 0 deletions tests/Symfony/Tests/Component/Form/FormTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,49 @@ public function testBindPostOrPutRequest($method)
unlink($path);
}

/**
* @dataProvider requestMethodProvider
*/
public function testBindPostOrPutRequestWithEmptyRootFormName($method)
{
$path = tempnam(sys_get_temp_dir(), 'sf2');
touch($path);

$values = array(
'name' => 'Bernhard',
'image' => array('filename' => 'foobar.png'),
'extra' => 'data',
);

$files = array(
'image' => array(
'error' => UPLOAD_ERR_OK,
'name' => 'upload.png',
'size' => 123,
'tmp_name' => $path,
'type' => 'image/png',
),
);

$request = new Request(array(), $values, array(), array(), $files, array(
'REQUEST_METHOD' => $method,
));

$form = $this->getBuilder('')->getForm();
$form->add($this->getBuilder('name')->getForm());
$form->add($this->getBuilder('image')->getForm());

$form->bindRequest($request);

$file = new UploadedFile($path, 'upload.png', 'image/png', 123, UPLOAD_ERR_OK);

$this->assertEquals('Bernhard', $form['name']->getData());
$this->assertEquals($file, $form['image']->getData());
$this->assertEquals(array('extra' => 'data'), $form->getExtraData());

unlink($path);
}

public function testBindGetRequest()
{
$values = array(
Expand All @@ -891,6 +934,29 @@ public function testBindGetRequest()
$this->assertEquals('Schussek', $form['lastName']->getData());
}

public function testBindGetRequestWithEmptyRootFormName()
{
$values = array(
'firstName' => 'Bernhard',
'lastName' => 'Schussek',
'extra' => 'data'
);

$request = new Request($values, array(), array(), array(), array(), array(
'REQUEST_METHOD' => 'GET',
));

$form = $this->getBuilder('')->getForm();
$form->add($this->getBuilder('firstName')->getForm());
$form->add($this->getBuilder('lastName')->getForm());

$form->bindRequest($request);

$this->assertEquals('Bernhard', $form['firstName']->getData());
$this->assertEquals('Schussek', $form['lastName']->getData());
$this->assertEquals(array('extra' => 'data'), $form->getExtraData());
}

public function testBindResetsErrors()
{
$form = $this->getBuilder()->getForm();
Expand Down Expand Up @@ -1024,6 +1090,24 @@ public function testGetErrorsAsStringDeep()
$this->assertEquals("name:\n ERROR: Error!\nfoo:\n No errors\n", $parent->getErrorsAsString());
}

public function testFormCanHaveEmptyName()
{
$form = $this->getBuilder('')->getForm();

$this->assertEquals('', $form->getName());
}

/**
* @expectedException Symfony\Component\Form\Exception\FormException
* @expectedExceptionMessage Form with empty name can not have parent form.
*/
public function testFormCannotHaveEmptyNameNotInRootLevel()
{
$parent = $this->getBuilder()
->add($this->getBuilder(''))
->getForm();
}

protected function getBuilder($name = 'name', EventDispatcherInterface $dispatcher = null)
{
return new FormBuilder($name, $this->factory, $dispatcher ?: $this->dispatcher);
Expand Down