Skip to content

Fix method tag for longer method names #203

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

Merged
merged 2 commits into from
Feb 12, 2020
Merged

Fix method tag for longer method names #203

merged 2 commits into from
Feb 12, 2020

Conversation

danrot
Copy link
Contributor

@danrot danrot commented Feb 10, 2020

I have added a testcase that currently fails, although it shouldn't. There seems to be a problem with @method annotations, when the method name gets a little bit longer. According to Regex101 it seems to result in a catastrophic backtrack, which gives me the feeling that this regex is too complicated. I tried to simplify it, but couldn't find any solution that generally worked.

The problem is this regex, if anybody has an idea how to improve it, I am happy to implement it.

@jaapio
Copy link
Member

jaapio commented Feb 10, 2020

Thanks for this test case, I cannot make any promises when this is fixed. But it looks like an interesting improvement first hand.

@danrot
Copy link
Contributor Author

danrot commented Feb 11, 2020

Just FYI: This is happening when using the Gedmo Doctrine Extensions, which will make in turn Prophecy fail when trying to mock one of these classes... So I could imagine that this error affects more people.

If anybody could give me a hint I might be able to investigate further.

Other question: If regular expressions seem to be so problematic in that regard, would it be ok to solve this with some PHP code instead?

@mvriel
Copy link
Member

mvriel commented Feb 11, 2020

Other question: If regular expressions seem to be so problematic in that regard, would it be ok to solve this with some PHP code instead?

That would need to be thoroughly benchmarked; this component uses regex's extensive as they are very performant. This library is used in a tight loop and for some consumers thousands of times; performance is essential.

@danrot
Copy link
Contributor Author

danrot commented Feb 11, 2020

Other question: If regular expressions seem to be so problematic in that regard, would it be ok to solve this with some PHP code instead?

That would need to be thoroughly benchmarked; this component uses regex's extensive as they are very performant. This library is used in a tight loop and for some consumers thousands of times; performance is essential.

But as far as I can see this call is failing because of performance issues (it seems just as if PHP stops execution of the regex, because it runs into some limit), as soon as the method name gets a little bit longer. And I am not talking about unreasonably long here, a few characters are already enough.

@mvriel
Copy link
Member

mvriel commented Feb 11, 2020

I cannot answer that question without diving into the code, for which I do not have time at this moment. I can only say, that as a design philosophy this library relies on regular expressions.

They are generally more performant than most PHP-based solutions that we have explored in the past and changing this to a purely based PHP solution should be benchmarked to prevent a performance regression.

I am currently assuming that the issue that you encounter has to do with an error in the regex; I may be proven wrong.

@danrot
Copy link
Contributor Author

danrot commented Feb 11, 2020

As said, I am also happy to fix this using regular expressions. However, I was not able to until now. So any tips are appreciated 🙂

@rvanvelzen
Copy link

Adjusting the regex like this should help.

@danrot
Copy link
Contributor Author

danrot commented Feb 11, 2020

@rvanvelzen Didn't know about possessive quantifiers so far! Thank you very much, all tests are succeeding now for me 🙂

@danrot danrot changed the title Add failing testcase for long method name Fix method tag for longer method names Feb 11, 2020
@mvriel
Copy link
Member

mvriel commented Feb 12, 2020

@rvanvelzen you are a true regex wizard; I am in awe of your skills :)

@mvriel mvriel merged commit 51b7739 into phpDocumentor:master Feb 12, 2020
@mvriel
Copy link
Member

mvriel commented Feb 12, 2020

@danrot thanks for reporting the issue and make this happen

@danrot danrot deleted the long-method-annotation branch February 12, 2020 06:31
@danrot
Copy link
Contributor Author

danrot commented Feb 12, 2020

@mvriel Happy to give back from time to time 🙂

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.

4 participants