[BUGFIX] Check existing Content-Type is in <head> #961
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
b99fea8
to
569a312
Compare
|
Theres a Psalm error I need to fix... |
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
569a312
to
14f63ee
Compare
|
Now fixed the Psalm errors. (I've not been running it locally because I get about 50 more errors that we don't get on the build server - not sure why, maybe different OS or PHP version - am using the same PHAR.) |
|
You'll probably need to edit the commit message and title before merging, as it includes your (@oliverklee) initial WIP commit with tests. |
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
14f63ee
to
57aa616
Compare
|
Last force-push was just fixing a typo in a comment, and adding a |
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
57aa616
to
3e6dff5
Compare
|
Now just moved exception handling to higher level for clarity. |
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
3e6dff5
to
5e1b0ec
Compare
oliverklee
requested changes
Apr 5, 2021
If a valid `Content-Type` `<meta>` element is present, DOM conversion will create a `<head>` element for it even without an explicit `<head>` tag in the HTML. However, to be valid, it must not be in the `<body>` element. As well as with an explicit `<body>` start tag, the `<body>` element also begins whenever a start tag for an element which cannot be in the `<head>` is encountered. This is now checked. Fixes #923.
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
5e1b0ec
to
678c546
Compare
oliverklee
requested changes
Apr 5, 2021
JakeQZ
force-pushed
the
bugfix/missing-head
branch
from
aa9cca6
to
3c47164
Compare
oliverklee
approved these changes
Apr 5, 2021
JakeQZ
added a commit
that referenced
this pull request
Apr 8, 2021
During review for #961, a regex change to capture the HTML before the `Content-Type` tag drew attention to a couple of edge cases that weren't explicitly tested for and would fail if there was a mistake in the regex pattern: - A newline before the `Content-Type` tag (would fail without the `PCRE_DOTALL`/`s` modifier - while caught by another test, `getDomDocumentWithNormalizedHtmlRepresentsTheGivenHtml`, that test was not specifically intended for this purpose); - A `Content-Type` tag in both the `<head>` and `<body>` (would fail without the lazy/`?` quantifier after `.*`). Tests have now been added to cover these.
JakeQZ
added a commit
that referenced
this pull request
Apr 8, 2021
During review for #961, a regex change to capture the HTML before the `Content-Type` tag drew attention to a couple of edge cases that weren't explicitly tested for and would fail if there was a mistake in the regex pattern: - A newline before the `Content-Type` tag (would fail without the `PCRE_DOTALL`/`s` modifier - while caught by another test, `getDomDocumentWithNormalizedHtmlRepresentsTheGivenHtml`, that test was not specifically intended for this purpose); - A `Content-Type` tag in both the `<head>` and `<body>` (would fail without the lazy/`?` quantifier after `.*`). Tests have now been added to cover these.
oliverklee
pushed a commit
that referenced
this pull request
Apr 8, 2021
During review for #961, a regex change to capture the HTML before the `Content-Type` tag drew attention to a couple of edge cases that weren't explicitly tested for and would fail if there was a mistake in the regex pattern: - A newline before the `Content-Type` tag (would fail without the `PCRE_DOTALL`/`s` modifier - while caught by another test, `getDomDocumentWithNormalizedHtmlRepresentsTheGivenHtml`, that test was not specifically intended for this purpose); - A `Content-Type` tag in both the `<head>` and `<body>` (would fail without the lazy/`?` quantifier after `.*`). Tests have now been added to cover these.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a valid
Content-Type<meta>element is present, DOM conversion willcreate a
<head>element for it even without an explicit<head>tag in theHTML.
However, to be valid, it must not be in the
<body>element. As well as withan explicit
<body>start tag, the<body>element also begins whenever astart tag for an element which cannot be in the
<head>is encountered. Thisis now checked.
Fixes #923.