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

Make Psalter add @throws annotation with properly namespaced exception #8480

Merged
merged 6 commits into from Sep 19, 2022

Conversation

d-claassen
Copy link
Contributor

Fixes #8467.

I've added the test provided in #8467, fixed that by prefixing the classname with a backslash and updated other tests.

I'm not certain if the fix for the exception class is now in the right place, and if there are possibly utilities to solve this. If there are, I'd be happy to apply them.

Running $ composer tests failed because of the same reasons for which the current main branch fails.

@d-claassen d-claassen changed the title Make Psalter add @throws annotation with properly namespaced exception Make Psalter add @throws annotation with properly namespaced exception Sep 13, 2022
@orklah
Copy link
Collaborator

orklah commented Sep 13, 2022

I'm surprised it fixed the issue. Did you find what was the cause of the problem for it to be solved by adding a slash?

@AndrolGenhald
Copy link
Collaborator

@orklah I haven't checked, but I'd assume it's currently adding @throws with the FQCN, but without the leading slash, so it's trying to resolve it from the current namespace instead of from the global namespace (could be wrong though). I'd prefer to fix it by using whatever namespace was used when throwing instead of adding a slash, but I haven't looked too much into this, so I don't know how difficult it would be.

@AndrolGenhald AndrolGenhald added release:fix The PR will be included in 'Fixes' section of the release notes PR: Need review labels Sep 14, 2022
@d-claassen
Copy link
Contributor Author

@orklah I did not dive deeper back into the origin of this issue. But I’m glad to hear that you find this an odd fix as well. I’ll have a look in the upcoming days to dive deeper into the original source.

@AndrolGenhald Adding the @throws annotation with the namespace as used when thrown will be complex. This also adds the annotation for exceptions thrown by methods that are being called within the method and are not caught. If that method lives in a different class in a different namespace, that will need to be resolved properly. By just defaulting to the FQCN it reduces a lot of complexity. I can add a unit test to show this behavior!

@AndrolGenhald
Copy link
Collaborator

This also adds the annotation for exceptions thrown by methods that are being called within the method and are not caught.

That makes sense, I hadn't considered that scenario.

I can add a unit test to show this behavior!

Yeah, please do. That way if anyone ever tries to get it to use already-used namespaces we'll have that test to make sure that part still works.

@d-claassen
Copy link
Contributor Author

d-claassen commented Sep 17, 2022

The FunctionDocblockManipulator receives the missing @throws exception classes from the FunctionLikeAnalyzer. I looked higher up the original source, and the class names without a leading slash seem to come all the way up from PhpParser\Node\Expr\New_. Since that seems to be relied upon more, I concluded that the right place to create an exception class with leading slash is the FunctionLikeAnalyzer and provide the right class name to the FunctionDocblockManipulator.

I found a usage of the TNamedObject::toNamespacedString in the FunctionLikeAnalyzer that did what I also needed. It also takes into account the source, so I've added an additional test to show that it takes relevant use statement into account. After applying these changes and removing my original "fix", I could also revert some change to the other tests that didn't need the leading slash per se.

So the end result that Psalter generates now is a lot nicer, which the tests show.
I think that the code change that I added is on the right level now.
I'm not sure if this the way to make that change, or if there are any common approaches I've missed.
I'm also not sure if I should use some property from the TNamedObject when adding the MissingThrowsDocblock to the IssueBuffer, instead of the full class name without the leading slash. Or if I should move this specific code to the if block specifically targeting Psalter.

@AndrolGenhald
Copy link
Collaborator

Looks great! Could you rebase on 4.x to fix the failing code style check?

@d-claassen d-claassen force-pushed the psalter-throws-namespaced-exception branch from 5c8ce92 to 3a6b709 Compare September 19, 2022 19:10
@d-claassen
Copy link
Contributor Author

@AndrolGenhald Done. Rebased & pushed

@orklah
Copy link
Collaborator

orklah commented Sep 19, 2022

Seems great! Thanks!

@orklah orklah merged commit f31f7be into vimeo:4.x Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Need review release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixing MissingThrowsDocblock adds relative class names
3 participants