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] Removing self-closing slash from <input> #47715

Merged
merged 1 commit into from Aug 1, 2023

Conversation

ThomasLandauer
Copy link
Contributor

Q A
Branch? 6.2
Bug fix? kinda
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR n/a

Switching form template from XHTML to HTML5 syntax.

https://validator.w3.org/nu/ says:

Warning: Self-closing tag syntax in text/html documents is widely discouraged; it’s unnecessary and interacts badly with other HTML features (e.g., unquoted attribute values). If you’re using a tool that injects self-closing tag syntax into all void elements, without any option to prevent it from doing so, then consider switching to a different tool.

I haven't searched for other occurrences; waiting for some feedback first.

@carsonbot carsonbot added this to the 6.2 milestone Sep 28, 2022
@ThomasLandauer ThomasLandauer changed the title Removing self-closing slash from <input> [Form] Removing self-closing slash from <input> Sep 28, 2022
ThomasLandauer added a commit to ThomasLandauer/symfony-docs that referenced this pull request Sep 29, 2022
I suggested the same for Symfony's templates, see symfony/symfony#47715
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 5, 2022
…ThomasLandauer)

This PR was merged into the 6.2 branch.

Discussion
----------

[Security] Removing self-closing slash from `<input>`s

I suggested the same for Symfony's templates, see symfony/symfony#47715

Commits
-------

c08a0a8 Removing self-closing slash from `<input>`s
@maxbeckers
Copy link
Contributor

Hi @ThomasLandauer, did you see, there are failed a lot of related tests. And there is a space before the > what's not needed i guess.

@fabpot fabpot modified the milestones: 6.2, 6.3 Nov 1, 2022
@fabpot
Copy link
Member

fabpot commented Dec 9, 2022

@ThomasLandauer Do you have time to fix the tests?

@ThomasLandauer
Copy link
Contributor Author

I'm not sure. The tests are based on DOMDocument::loadXML(), which probably needs to be switched to ::loadHTML()?! But there's some stuff going on that I don't understand, e.g.:

// Wrap in <root> node so we can load HTML with multiple tags at
// the top level
$dom->loadXML('<root>'.$html.'</root>');

So if I should do it, I'd need somebody to take me by the hand...

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Tests fixed.
I also removed all /> from the profiler and error pages.

@@ -72,6 +72,9 @@ protected function assertXpathNodeValue(\DOMElement $element, $expression, $node
protected function assertMatchesXpath($html, $expression, $count = 1)
{
$dom = new \DOMDocument('UTF-8');

$html = preg_replace('/(<input [^>]+)(?<!\/)>/', '$1/>', $html);
Copy link
Member

Choose a reason for hiding this comment

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

Here is the change that helps get tests green.

@carsonbot carsonbot changed the title [Form] Removing self-closing slash from <input> Removing self-closing slash from <input> Aug 1, 2023
@carsonbot carsonbot changed the title Removing self-closing slash from <input> [Form] Removing self-closing slash from <input> Aug 1, 2023
@fabpot
Copy link
Member

fabpot commented Aug 1, 2023

Thank you @ThomasLandauer.

@fabpot fabpot merged commit 5fcaa74 into symfony:6.4 Aug 1, 2023
3 of 9 checks passed
@ThomasLandauer ThomasLandauer deleted the patch-2 branch August 1, 2023 19:02
This was referenced Oct 21, 2023
nicolas-grekas pushed a commit that referenced this pull request Jan 29, 2024
nicolas-grekas added a commit that referenced this pull request Jan 29, 2024
…#47715 (mpdude)

This PR was merged into the 6.4 branch.

Discussion
----------

[Form] Use self-closing `<input />` syntax again, reverting #47715

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #53649
| License       | MIT

This PR reverts #47715 and puts the trailing slash back into `<input .../>` markup in the form themes.

The reasons are outlined in detail in #53649. Basically, the trailing slash in void elements like `<input>` is not required in HTML 5, yet it is perfectly valid and compliant with specs.

Writing markup that way is required to parse and process it with an XML parser, like `libxml2` underlying all of the PHP DOM, XML etc. extensions.

HTML 5 capable parser support has been added to PHP in php/php-src#12111 and will be available with PHP 8.4. So, we might want to remove the trailing slashes once Symfony requires at least PHP 8.4.

Commits
-------

8c92200 Revert #47715
nicolas-grekas added a commit that referenced this pull request Jan 29, 2024
* 6.4: (21 commits)
  [ErrorHandler] Add missing self-closing tags on link elements
  Fix merge (bis)
  Fix merge
  Add missing return type
  [FrameworkBundle] ConfigBuilderCacheWarmer should be non-optional
  [HttpClient] Fix pausing responses before they start when using curl
  [Notifier] Updated the NTFY notifier to run without a user parameter
  [Translation] Fix constant domain resolution in PhpAstExtractor
  separate child and parent context in NotificationEmail on writes
  [Mailer] [Mailgun] Fix sender header encoding
  do not overwrite the cache key when it is false
  [Mailer] [Scaleway] Fix attachment handling
  [Mailer] Throw TransportException when unable to read from socket
  [Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context when creating child contexts
  Revert #47715
  [HttpClient] Fix error chunk creation in passthru
  Adjusting and removing the 'review' attribute from the pt_br translation XML.
  [DependencyInjection] Fix loading all env vars from secrets when only a subset is needed
  Fix option filenameMaxLength to the File constraint (Image)
  [Serializer] Take unnamed variadic parameters into account when denormalizing
  ...
nicolas-grekas added a commit that referenced this pull request Jan 29, 2024
* 7.0: (21 commits)
  [ErrorHandler] Add missing self-closing tags on link elements
  Fix merge (bis)
  Fix merge
  Add missing return type
  [FrameworkBundle] ConfigBuilderCacheWarmer should be non-optional
  [HttpClient] Fix pausing responses before they start when using curl
  [Notifier] Updated the NTFY notifier to run without a user parameter
  [Translation] Fix constant domain resolution in PhpAstExtractor
  separate child and parent context in NotificationEmail on writes
  [Mailer] [Mailgun] Fix sender header encoding
  do not overwrite the cache key when it is false
  [Mailer] [Scaleway] Fix attachment handling
  [Mailer] Throw TransportException when unable to read from socket
  [Serializer] Rewrite `AbstractObjectNormalizer::createChildContext()` to use the provided `cache_key` from original context when creating child contexts
  Revert #47715
  [HttpClient] Fix error chunk creation in passthru
  Adjusting and removing the 'review' attribute from the pt_br translation XML.
  [DependencyInjection] Fix loading all env vars from secrets when only a subset is needed
  Fix option filenameMaxLength to the File constraint (Image)
  [Serializer] Take unnamed variadic parameters into account when denormalizing
  ...
This was referenced Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants