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

[junit formatter] Do not escape the error message twice #3087

Merged
merged 5 commits into from
May 30, 2024

Conversation

mousetail
Copy link
Contributor

@mousetail mousetail commented May 21, 2024

Currently, the junit formatter escapes the error message twice. This makes it hard to read.

The message is escaped once inside JunitErrorFormatter::createTextCase and one in JunitErrorFormatter::formatErrors. Only of of these is needed.

IMHO this should be the responsibility of createTestCase so I removed it from formatErrors

Previous Output

<failure type="ERROR"
    message="Parameter #4 $options of method App\Auth\Provider::createRequest() expects array&amp;lt;string, mixed&amp;gt;, array&amp;lt;int, string&amp;gt; given."/>

New Output

<failure type="ERROR"
    message="Parameter #4 $options of method App\Auth\Provider::createRequest() expects array&lt;string, mixed&gt;, array&lt;int, string&gt; given."/>

@ondrejmirtes
Copy link
Member

Please show this problem in a test that fails without the changes in src/.

@mousetail
Copy link
Contributor Author

@ondrejmirtes Tests added

@mousetail
Copy link
Contributor Author

Not exactly sure why generic errors in the table formatter are not rendering properly in PHP <8.1 but row specific errors are. The code between the two cases seems identical

https://github.com/mousetail/phpstan-src/blob/patch-1/src/Command/ErrorFormatter/TableErrorFormatter.php#L131

There is some odd printing HTML to console stuff going on? That might explain the missing generic if it's interpreted as a HTML tag. Doesn't explain why it works properly for the file specific error case or in PHP 8.1-8.3 though.

In either case it doesn't seem related to this issue, though it will need to be fixed before this can be merged.

@mousetail
Copy link
Contributor Author

Tests are fixed but not sure why some of the the integration tests are failing. The failures seem unrelated to the changes

@mousetail
Copy link
Contributor Author

@ondrejmirtes Anything more necessary to get this merged?

@ondrejmirtes ondrejmirtes merged commit 39f5f45 into phpstan:1.11.x May 30, 2024
441 of 445 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@mousetail mousetail deleted the patch-1 branch May 31, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants