Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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.
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
base:7.4
Are you sure you want to change the base?
Conversation
8b45fe1 to3239eb8ComparePR is ready. doctrine-bridge test fails because it still uses old |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, just some tweaks please
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…sispts)This PR was merged into the 5.4 branch.Discussion----------[DoctrineBridge] Drop a test case in `EntityTypeTest`| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->| License | MITThis PR will drop a test case as suggested by `@nicolas`-grekas. See:#53981 (comment)Commits-------f6506aa Drop test case
d6740c2 to0312392Compare0312392 to534ea18CompareRebased and ready |
| */ | ||
| 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we fordbidisindex now? This is a BC break if I don’t miss anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Becauseisindex is not allowed in HTML5.
Yeah, it's a BC break. Any idea how to handle this case?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 useisindex without issue. The HTML restriction would only forbid us to generate afull name equal toisindex, whileroot[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)
There was a problem hiding this comment.
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 onlyisindex, but also_charset_, seehttps://html.spec.whatwg.org/multipage/form-control-infrastructure.html#attr-fe-name
AnASCII case-insensitive match for the name
_charset_is special: if used as the name of aHidden control with novalue attribute, then during submission thevalue attribute is automatically given a value consisting of the submission character encoding.
| // 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. |
There was a problem hiding this comment.
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 ?
| ['isINDEX'], | ||
| // This value shouldn't be allowed. | ||
| // However, many tests in Form component require empty name |
There was a problem hiding this comment.
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 likefield1 and notroot[field1].
| // 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'); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
@asispts This PR looks stale, are you still willing to work on this PR? |
Uh oh!
There was an error while loading.Please reload this page.
Changes in this PR:
testNameAcceptsOnlyNamesValidAsIdsInHtml4has been split into 2 functions:testInvalidFormInputNameandtestValidFormInputName,isindexas an input name.