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

ISSUE-5962 Fixed wrong line number for @method annotations #7157

Merged
merged 1 commit into from Dec 15, 2021

Conversation

nowaja
Copy link
Contributor

@nowaja nowaja commented Dec 14, 2021

Resolves #5962

@orklah
Copy link
Collaborator

orklah commented Dec 14, 2021

Why did you change min php version? Was there a feature you needed?

@orklah
Copy link
Collaborator

orklah commented Dec 14, 2021

If this is about CI failure on isStatic, it's unrelated, it will be fixed this afternoon

@nowaja
Copy link
Contributor Author

nowaja commented Dec 14, 2021

If this is about CI failure on isStatic, it's unrelated, it will be fixed this afternoon

Yes, i tried if that could be related. Reverted the change. Thanks.

@orklah
Copy link
Collaborator

orklah commented Dec 14, 2021

Can you explain why you changed something here instead of when we report an issue? It may be the correct way, but that would not have been my first instinct

@weirdan
Copy link
Collaborator

weirdan commented Dec 15, 2021

I'm pretty sure it's a correct approach. We're in foreach over method entries, and the code as it was was creating virtual methods with the same offset, which can't be right.

Fixing it at the emit point would likely require reparsing the doc comment at that point, potentially multiple times. Which, if you ask, would be kinda silly and wasteful.

@nowaja
Copy link
Contributor Author

nowaja commented Dec 15, 2021

Thank you @weirdan, you were faster. I also tried different solution but exactly what you mentioned had happened. I had to change multiple places, it was really hard to follow. This solution seemed the best.

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Dec 15, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 15, 2021

Thanks!

@orklah orklah merged commit 16c0496 into vimeo:master Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Wrong line numbers reported in some cases
3 participants