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

Add some more attributes test #944

Closed
wants to merge 2 commits into from
Closed

Conversation

MrSpoocy
Copy link
Contributor

Related to #943, here's a few more test. One of them also fails at the moment because there seems to be a bug.

@@ -43,7 +43,7 @@ final class RegexHelper
public const PARTIAL_TAGNAME = '[a-z][a-z0-9-]*';
public const PARTIAL_BLOCKTAGNAME = '(?:address|article|aside|base|basefont|blockquote|body|caption|center|col|colgroup|dd|details|dialog|dir|div|dl|dt|fieldset|figcaption|figure|footer|form|frame|frameset|h1|head|header|hr|html|iframe|legend|li|link|main|menu|menuitem|nav|noframes|ol|optgroup|option|p|param|section|source|summary|table|tbody|td|tfoot|th|thead|title|tr|track|ul)';
public const PARTIAL_ATTRIBUTENAME = '[a-z_:][a-z0-9:._-]*';
public const PARTIAL_UNQUOTEDVALUE = '[^"\'=<>`\x00-\x20]+';
public const PARTIAL_UNQUOTEDVALUE = '[^"\'=<>}`\x00-\x20]+';
Copy link
Member

Choose a reason for hiding this comment

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

This constant is also used to parse HTML and according to the spec a } is allowed in unquoted values:

Attributes are placed inside the start tag, and consist of a name and a value, separated by an "=" character. The attribute value can remain unquoted if it doesn't contain ASCII whitespace or any of " ' ` = < or >. Otherwise, it has to be quoted using either single or double quotes. The value, along with the "=" character, can be omitted altogether if the value is the empty string.

So we'll need to find another way to fix the issue you found.

@colinodell colinodell mentioned this pull request Oct 30, 2022
@colinodell
Copy link
Member

Thanks for adding these tests! They were super helpful in pinpointing the cause of this issue. I'm going to merge an alternate solution (that includes your new tests) in #945.

@colinodell colinodell closed this Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants