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

🐛 Fix yaml number rendering #179

Merged
merged 1 commit into from
Sep 6, 2024
Merged

🐛 Fix yaml number rendering #179

merged 1 commit into from
Sep 6, 2024

Conversation

homersimpsons
Copy link
Contributor

@homersimpsons homersimpsons commented Aug 1, 2024

Fixes #177

I copied the yaml.json file from https://github.com/scrivo/highlight.php/blob/d1c6b0956a2b0d3efc137e4d8c5bf5c5e220bac1/src/Highlight/languages/yaml.json#L84 and updated the "number" regex to add _ where I think the symfony parser allow them:

- "begin": "(-?)(\\b0[xX][a-fA-F0-9]+|(\\b\\d+(\\.\\d*)?|\\.\\d+)([eE][-+]?\\d+)?)\\b"
+ "begin": "(-?)(\\b0[xX][a-fA-F0-9_]+|(\\b\\d[\\d_]*(\\.[\\d_]*)?|\\.\\d[\\d_]*)([eE][-+]?\\d[\\d_]*)?)\\b"

There may be additional places where the symfony parser allow the _ for numbers.

I also updated the tests to add the example from the documentation.

Copy link
Contributor

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

Overwriting the syntax file in this project is indeed the most sensible option (see also my more lengthy response on this matter: #176 (comment)).

The highlight.js syntax file has support for some more number types (especially timestamps, datetimes, etc.). Maybe we can also add those regexes (as a separate number item)? (given the Yaml component supports date strings)

@homersimpsons
Copy link
Contributor Author

The highlight.js syntax file has support for some more number types (especially timestamps, datetimes, etc.). Maybe we can also add those regexes (as a separate number item)? (given the Yaml component supports date strings)

I can add those, but are they supported by symfony/yaml's parser? I saw we were parsing dates but not sure about the exact syntax supported.

@homersimpsons
Copy link
Contributor Author

homersimpsons commented Aug 9, 2024

Just tested it. Their regex is pretty eager:

  • It allows any year of 4 digit, I sticked with 2024 (I may have found a symfony bug without this constraint)
  • It allows having a fractional time part without having a time
var_dump(Yaml::parse(<<<YAML
v0: 2024-12-07 23:44:39.35677451141299410 	    	    		   		 	 	 	  	 	    		   			  		 			 			 				  	 		
v1: 2024-02-3.671	-11
v2: 2024-82T3:66:07		+36
v3: 2024-07-05t6:39:20	 	   		 	        	
v4: 2024-01-30.000				 	 	  	  				 							 	 	 	  	 	 			      			 				  	 	     	   	   	 		    	
v5: 2024-09-03 			 					  	   			    	   	  	    		  	
YAML
, Yaml::PARSE_DATETIME));

will give the following, we can see that symfony won't handle all possible "dates":

array(6) {
  'v0' =>
  class DateTimeImmutable#4 (3) {
    public $date =>
    string(26) "2024-12-07 23:44:39.356774"
    public $timezone_type =>
    int(3)
    public $timezone =>
    string(3) "UTC"
  }
  'v1' =>
  string(17) "2024-02-3.671     -11"
  'v2' =>
  string(20) "2024-82T3:66:07           +36"
  'v3' =>
  class DateTimeImmutable#5 (3) {
    public $date =>
    string(26) "2024-07-05 06:39:20.000000"
    public $timezone_type =>
    int(3)
    public $timezone =>
    string(3) "UTC"
  }
  'v4' =>
  string(14) "2024-01-30.000"
  'v5' =>
  class DateTimeImmutable#6 (3) {
    public $date =>
    string(26) "2024-09-03 00:00:00.000000"
    public $timezone_type =>
    int(3)
    public $timezone =>
    string(3) "UTC"
  }
}

@wouterj Should I still use their regex or use the one from symfony/yaml?

xabbuh added a commit to symfony/symfony that referenced this pull request Aug 12, 2024
This PR was merged into the 5.4 branch.

Discussion
----------

[Yaml] 🐛 throw ParseException on invalid date

| Q             | A
| ------------- | ---
| Branch?       | 5.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | None <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

(found in symfony-tools/docs-builder#179)

When parsing the following yaml:
```
date: 6418-75-51
```

`symfony/yaml` will throw an exception:
```
$ php main.php
PHP Fatal error:  Uncaught Exception: Failed to parse time string (6418-75-51) at position 6 (5): Unexpected character in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php:714
Stack trace:
#0 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(714): DateTimeImmutable->__construct()
#1 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(312): Symfony\Component\Yaml\Inline::evaluateScalar()
#2 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(80): Symfony\Component\Yaml\Inline::parseScalar()
#3 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(790): Symfony\Component\Yaml\Inline::parse()
#4 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(341): Symfony\Component\Yaml\Parser->parseValue()
#5 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(86): Symfony\Component\Yaml\Parser->doParse()
#6 /tmp/symfony-yaml/vendor/symfony/yaml/Yaml.php(77): Symfony\Component\Yaml\Parser->parse()
#7 /tmp/symfony-yaml/main.php(8): Symfony\Component\Yaml\Yaml::parse()
#8 {main}
  thrown in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php on line 714
```

This is because the "month" is invalid. Fixing the "month" will trigger about the same issue because the "day" would be invalid.

With the current change it will throw a `ParseException`.

Commits
-------

6d71a7e 🐛 throw ParseException on invalid date
@homersimpsons
Copy link
Contributor Author

homersimpsons commented Sep 6, 2024

@wouterj I used the regex from symfony/yaml. I had to clean it up so it works in JS (removed the capture groups), and I also removed the non-capture groups that are unused.

The failure is probably not linked to my changes. I rebased on main, but I still have the error.

Copy link
Collaborator

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks @homersimpsons

Copy link
Contributor

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Nice, this is looking great!

Failure is indeed not related to this PR.

@javiereguiluz
Copy link
Collaborator

Merged! Thanks

@javiereguiluz javiereguiluz merged commit 4443a68 into symfony-tools:main Sep 6, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Minor YAML highlighting issue
3 participants