Fixes #6255 - removed regex for token parsing #6286
Conversation
…o hotfix/6255 bring up to date
|
Hm, there appears to be some issues with PHP 5.5 and 5.6. I'll need to investigate this further.. |
|
@steverhoades switching to tokenizer-based reads is absolutely a plus - still didn't have time to look over it though. I obviously see a lot of space for edge cases (as usual) |
|
@Ocramius agreed. I'd like to get as many of those edge cases covered if possible. I can see there are missing token checks for newer PHP versions that will need to be added as well. Let me know what you come up with when you have time and I'll make sure and get them covered. |
|
Weirdness with travis? unrelated test is failing - |
|
@steverhoades yes, that test fails randomly and is very annoying :\ |
|
I'm marking this for 2.4, as the code changes are quite large (switching from regex to token parser will likely have funny consequences) |
|
See #6255 |
| return $cacheKey; | ||
| } | ||
|
|
||
| // //TODO - would it be better to define the binding like this? |
There was a problem hiding this comment.
@steverhoades any plans for this?
There was a problem hiding this comment.
@Ocramius No, that is included as part of the test actually. It's not meant to be an actual TODO. Perhaps I should have referenced the issue from which I used to create the test case :(
Essentially it's testing that the Reflection class properly handles comment parsing. For instance in this case, it should ignore the __prototype function as it's commented out.
|
Scheduling for 2.3.2, as this is technically a bugfix. |
|
Rebased/merged manually. Thanks @steverhoades, this one is quite an improvement! |
It appears that the regex expression is unreliable, I have added a token parsing mechanism in it's place. Please let me know if there are any test cases that we should add to this.