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: Explicit @psalm-param handling #39

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

provokateurin
Copy link
Member

Fixes #32

Both @param and @psalm-param were already parsed, but if both are present only the first was used. Now it considers both if both are present.

@nickvergessen
Copy link
Member

nickvergessen commented Dec 6, 2023

This breaks Talk again:

PHP Fatal error:  Uncaught Exception: Panic: BreakoutRoom#configureBreakoutRooms: Unable to resolve OpenAPI type for type 'PHPStan\PhpDocParser\Ast\Type\ConstTypeNode'
 in /home/nickv/Nextcloud/Repositories/openapi-extractor/src/Logger.php:40
Stack trace:
#0 /home/nickv/Nextcloud/Repositories/openapi-extractor/src/OpenApiType.php(211): OpenAPIExtractor\Logger::panic()
#1 /home/nickv/Nextcloud/Repositories/openapi-extractor/src/OpenApiType.php(98): OpenAPIExtractor\OpenApiType::resolve()
#2 /home/nickv/Nextcloud/Repositories/openapi-extractor/src/ControllerMethodParameter.php(21): OpenAPIExtractor\OpenApiType::resolve()
#3 /home/nickv/Nextcloud/Repositories/openapi-extractor/src/ControllerMethod.php(143): OpenAPIExtractor\ControllerMethodParameter->__construct()
#4 /home/nickv/Nextcloud/Repositories/openapi-extractor/generate-spec(351): OpenAPIExtractor\ControllerMethod::parse()
#5 {main}
  thrown in /home/nickv/Nextcloud/Repositories/openapi-extractor/src/Logger.php on line 40

I moved the "unsupported const" to psalm types to not break the openapi definition while the extractor does not support it and listed the plain list of integers on the normal type:
https://github.com/nextcloud/spreed/blob/4c5128dc22af05217df9e9d044515dbf921e05dc/lib/Controller/BreakoutRoomController.php#L62-L63

Can we catch the exception and ignore it if another type was given, until constants are understood?

@provokateurin
Copy link
Member Author

Not great, but I guess the only way right now. Let's still print it as a warning though.

@nickvergessen
Copy link
Member

nickvergessen commented Dec 6, 2023

Not great, but I guess the only way right now.

The problem of being in the middle of 2 evolutions :)

Let's still print it as a warning though.

Sounds okay for me atm

@provokateurin provokateurin force-pushed the fix/explicit-psalm-param-handling branch from ce8de1d to 89c827e Compare December 6, 2023 14:43
@provokateurin
Copy link
Member Author

Testing this with Talk now shows some unimplemented types like non-negative-int 🎉

@provokateurin provokateurin force-pushed the fix/explicit-psalm-param-handling branch from 89c827e to 2303976 Compare December 6, 2023 15:09
src/ControllerMethod.php Show resolved Hide resolved
@nickvergessen
Copy link
Member

So time to do the fixups and merge?

Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
@provokateurin provokateurin force-pushed the fix/explicit-psalm-param-handling branch from 65eaab6 to 12faea2 Compare December 7, 2023 10:29
@provokateurin provokateurin merged commit 083417a into main Dec 7, 2023
1 check passed
@provokateurin provokateurin deleted the fix/explicit-psalm-param-handling branch December 7, 2023 10:30
@provokateurin
Copy link
Member Author

Ah too bad I forgot to remove the stacktrace from the log

@nickvergessen
Copy link
Member

Ah too bad I forgot to remove the stacktrace from the log

We can remove it when we add the verbose flag

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.

Parse @psalm-* params as well
2 participants