Skip to content

[Form] Allow more permissive form input name and ID #53981

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

Open
wants to merge 5 commits into
base: 7.4
Choose a base branch
from
+55 −28
Open

[Form] Allow more permissive form input name and ID #53981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
76170c1
Allow more permissive form input name
asispts Feb 17, 2024
673b0b7
Allow leading underscore and digits in Form ID
asispts Feb 17, 2024
1001812
Update changelog
asispts Feb 19, 2024
02de755
Fix tests
asispts Feb 20, 2024
534ea18
Use an array instead of yield
asispts Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/Form/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ CHANGELOG
* Deprecate not configuring the `default_protocol` option of the `UrlType`, it will default to `null` in 8.0
* Add a `keep_as_list` option to `CollectionType`
* Add a new `model_type` option to `MoneyType`, to be able to cast the transformed value to `integer`
* Allow more permissive form input name and id

7.0
---
Expand Down
5 changes: 0 additions & 5 deletions src/Symfony/Component/Form/Extension/Core/Type/BaseType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,6 @@ public function buildView(FormView $view, FormInterface $form, array $options):
$id = \is_string($options['form_attr']) ? $options['form_attr'] : $name;
$fullName = $name;
$uniqueBlockPrefix = '_'.$blockName;

// Strip leading underscores and digits. These are allowed in
// form names, but not in HTML4 ID attributes.
// https://www.w3.org/TR/html401/struct/global#adef-id
$id = ltrim($id, '_0123456789');
Copy link
Member

Choose a reason for hiding this comment

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

should be kept IMO as written in #53976 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

If this change is kept (which I think is good - see my answer at #53976 (comment) ), then it's a BC break and therefore should target v8.0.

}

$blockPrefixes = [];
Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/Form/FormConfigBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ public function setIsEmptyCallback(?callable $isEmptyCallback): static
final public static function validateName(?string $name): void
{
if (!self::isValidName($name)) {
throw new InvalidArgumentException(sprintf('The name "%s" contains illegal characters. Names should start with a letter, digit or underscore and only contain letters, digits, numbers, underscores ("_"), hyphens ("-") and colons (":").', $name));
throw new InvalidArgumentException(sprintf('The name "%s" contains illegal characters or equals to "isindex". Names should only contain letters, digits, underscores ("_"), hyphens ("-") and colons (":").', $name));
}
}

Expand All @@ -642,12 +642,12 @@ final public static function validateName(?string $name): void
* A name is accepted if it
*
* * is empty
* * starts with a letter, digit or underscore
* * contains only letters, digits, numbers, underscores ("_"),
* hyphens ("-") and colons (":")
* * is not equal to "isindex"
*/
final public static function isValidName(?string $name): bool
{
return '' === $name || null === $name || preg_match('/^[a-zA-Z0-9_][a-zA-Z0-9_\-:]*$/D', $name);
return ('' === $name || null === $name || preg_match('/^[a-zA-Z0-9_\-:]*$/D', $name)) && 'isindex' !== $name;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we fordbid isindex now? This is a BC break if I don’t miss anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because isindex is not allowed in HTML5.

Yeah, it's a BC break. Any idea how to handle this case?

Copy link
Member

Choose a reason for hiding this comment

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

Let‘s deprecate it in 7.1 and don’t accept it in 8.0.

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 this is wrong though. Most of the Symfony form names could use isindex without issue. The HTML restriction would only forbid us to generate a full name equal to isindex, while root[isindex] is perfectly valid in HTML (remember that the Symfony form name is not used directly as the HTML name, but is involved in generating it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just adding that it's not only isindex, but also _charset_, see https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#attr-fe-name

An ASCII case-insensitive match for the name _charset_ is special: if used as the name of a Hidden control with no value attribute, then during submission the value attribute is automatically given a value consisting of the submission character encoding.

}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/Form/Tests/ButtonBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public function testValidNames($name)
public function testNameContainingIllegalCharacters()
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('The name "button[]" contains illegal characters. Names should start with a letter, digit or underscore and only contain letters, digits, numbers, underscores ("_"), hyphens ("-") and colons (":").');
$this->expectExceptionMessage('The name "button[]" contains illegal characters or equals to "isindex".');

$this->assertInstanceOf(ButtonBuilder::class, new ButtonBuilder('button[]'));
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2084,12 +2084,12 @@ public function testPassIdAndNameToView()
$this->assertEquals('name', $view->vars['full_name']);
}

public function testStripLeadingUnderscoresAndDigitsFromId()
public function testAllowLeadingUnderscoresAndDigitsFromId()
{
$view = $this->factory->createNamed('_09name', static::TESTED_TYPE, null)
->createView();

$this->assertEquals('name', $view->vars['id']);
$this->assertEquals('_09name', $view->vars['id']);
$this->assertEquals('_09name', $view->vars['name']);
$this->assertEquals('_09name', $view->vars['full_name']);
}
Expand Down
65 changes: 48 additions & 17 deletions src/Symfony/Component/Form/Tests/FormConfigTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use PHPUnit\Framework\TestCase;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\Form\Exception\InvalidArgumentException;
use Symfony\Component\Form\FormConfigBuilder;
use Symfony\Component\Form\NativeRequestHandler;

Expand All @@ -21,56 +22,86 @@
*/
class FormConfigTest extends TestCase
{
public static function getHtml4Ids()
public static function provideInvalidFormInputName(): iterable
{
return [
['isindex'],
['#'],
['a#'],
['a$'],
['a%'],
['a '],
["a\t"],
["a\n"],

// Periods are allowed by the HTML4 spec, but disallowed by us
// because they break the generated property paths
['a.'],
];
}

/**
* @dataProvider provideInvalidFormInputName
*/
public function testInvalidFormInputName(string $name)
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage(sprintf('The name "%s" contains illegal characters or equals to "isindex". Names should only contain letters, digits, underscores ("_"), hyphens ("-") and colons (":").', $name));

new FormConfigBuilder($name, null, new EventDispatcher());
}

public static function provideValidFormInputName(): iterable
{
return [
['z0'],
['A0'],
['A9'],
['Z0'],
['#', 'Symfony\Component\Form\Exception\InvalidArgumentException'],
['a#', 'Symfony\Component\Form\Exception\InvalidArgumentException'],
['a$', 'Symfony\Component\Form\Exception\InvalidArgumentException'],
['a%', 'Symfony\Component\Form\Exception\InvalidArgumentException'],
['a ', 'Symfony\Component\Form\Exception\InvalidArgumentException'],
["a\t", 'Symfony\Component\Form\Exception\InvalidArgumentException'],
["a\n", 'Symfony\Component\Form\Exception\InvalidArgumentException'],
['a-'],
['a_'],
['a:'],
// Periods are allowed by the HTML4 spec, but disallowed by us
// because they break the generated property paths
['a.', 'Symfony\Component\Form\Exception\InvalidArgumentException'],

// Contrary to the HTML4 spec, we allow names starting with a
// number, otherwise naming fields by collection indices is not
// possible.
// For root forms, leading digits will be stripped from the
// "id" attribute to produce valid HTML4.
Copy link
Member

Choose a reason for hiding this comment

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

is this comment still accurate ?

['0'],
['9'],

// Contrary to the HTML4 spec, we allow names starting with an
// underscore, since this is already a widely used practice in
// Symfony.
// For root forms, leading underscores will be stripped from the
// "id" attribute to produce valid HTML4.
['_'],

// Integers are allowed
[0],
[123],

// NULL is allowed
[null],

// Allowed in HTML 5 specification
// See: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#attr-fe-name
['_charset_'],
['-x'],
[':x'],
['isINDEX'],

// This value shouldn't be allowed.
// However, many tests in Form component require empty name
Copy link
Member

Choose a reason for hiding this comment

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

This comment is wrong to me. The fullname of a non-compound field should not be empty (as HTML form inputs will need a non-empty name). But this is not what is validated by the validation you run here.

For instance, a perfectly valid use case is to have an empty name for the root form, so that names of fields look like field1 and not root[field1].

[''],
];
}

/**
* @dataProvider getHtml4Ids
* @dataProvider provideValidFormInputName
*/
public function testNameAcceptsOnlyNamesValidAsIdsInHtml4($name, $expectedException = null)
public function testValidFormInputName(string|int|null $name)
{
if (null !== $expectedException) {
$this->expectException($expectedException);
}

$formConfigBuilder = new FormConfigBuilder($name, null, new EventDispatcher());

$this->assertSame((string) $name, $formConfigBuilder->getName());
Expand Down