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 return type JsonFormatter::normalizeException() #1924

Closed
wants to merge 3 commits into from

Conversation

ro0NL
Copy link

@ro0NL ro0NL commented Nov 13, 2024

I dont see how normalizeException can return a string.

@Seldaek
Copy link
Owner

Seldaek commented Nov 13, 2024

See the build failure. It is done like that to allow the child class (LineFormatter) to override this. It's very difficult to have strict types for NormalizerFormatter as it is extended for various different use cases.

@ro0NL ro0NL changed the title Fix return type normalizeException Fix return type JsonFormatter::normalizeException() Nov 13, 2024
@@ -216,7 +216,7 @@ protected function normalize(mixed $data, int $depth = 0): mixed
* Normalizes given exception with or without its own stack trace based on
* `includeStacktraces` property.
*
* @return array<string, string|int|array<string|int|array<string>>>|string
* @return array<array-key, string|int|array<string|int|array<string>>>
Copy link
Author

Choose a reason for hiding this comment

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

keys might be non-string when max depth exceeds, and maybe in case of JsonSerializable

@ro0NL
Copy link
Author

ro0NL commented Nov 13, 2024

@Seldaek got ya. updated.

@Seldaek
Copy link
Owner

Seldaek commented Nov 13, 2024

Sorry not sure what you updated? :) Still failing

@ro0NL
Copy link
Author

ro0NL commented Nov 13, 2024

yeah ok. i dont feel like doing the phpstan bookkeeping. sorry.

we've casted JsonFormatter::normalize on our side.

@ro0NL ro0NL closed this Nov 13, 2024
@ro0NL ro0NL deleted the patch-1 branch November 13, 2024 12:02
@Seldaek
Copy link
Owner

Seldaek commented Nov 13, 2024

Yeah that's fine by me.. I don't know which is best here tbh, we gotta ignore some errors somewhere, but maybe for lib consumers it would be better if we did it like you had and ignore the JsonFormatter extension error. But I guess your array-key change should be applied still right? Is array-key equivalent to string|int? I can look at doing the change and updating the baseline.

@ro0NL
Copy link
Author

ro0NL commented Nov 13, 2024

psalm describes it as

array-key is the supertype (but not a union) of int and string.

so yes just a union in practice

Seldaek added a commit that referenced this pull request Nov 17, 2024
@Seldaek
Copy link
Owner

Seldaek commented Nov 17, 2024

OK I think this works e940004

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