Workaround PHP compiler globals storing doc_comments #9
Conversation
The PHP parser stores the last docblock comment it saw in a compiler global. When the compiler next sees something that might conceivably have a docblock comment, the thing becomes the owner of the comment, and the global is cleared. Unfortunately the parser does not clear state between parses. A call to token_get_all() on something containing a docblock comment will cause the global to be set. The next file to be compiled by an include() will start off with that global, and this (under some circumstances) can cause the comment to be erroneously associated with another class. This workaround ensures that after a call to token_get_all() we reset the parser to a state where the doc_comment compiler global contains an empty comment, and is therefore "safe" for us. This is only a workaround: With this fix we have to hope that any other uses of token_get_all() in the user's codebase are also similarly protected. IMO this is a PHP bug (for which I shall raise a bug report), and should, long term, be fixed there.
| // parser sees since PhpParser called token_get_all() on the intro to ClassWithClassAnnotationOnly. | ||
| // Our test class cannot be in a namespace (some versions of PHP reset the doc_comment compiler global when | ||
| // you hit a namespace declaration), so cannot be autoloaded. | ||
| require(__DIR__ . '/Fixtures/ClassNoNamespaceNoComment.php'); |
There was a problem hiding this comment.
you should use require_once to be safe against fatal errors. And please remove braces for the require call.
There was a problem hiding this comment.
If the file has already been included prior to this point it invalidates the test; We need the PHP compiler to run over the file at exactly this point in order to produce the problem.
Without adding some overly complex auto-generation of class files, a compromise would be switching to _once, but adding an assert so that the test fails if the file has been included early. I'll do that and remove the braces.
To make sure our test is valid we do, however, need to fail the test if someone has already managed to require the file.
| { | ||
| // If someone has already included our main test fixture this test is invalid. It's important that our require | ||
| // causes this file to be parsed and compiled at a certain point. | ||
| $this->assertFalse(class_exists('Doctrine_Tests_Common_Annotations_Fixtures_ClassNoNamespaceNoComment'), 'Test invalid if class has already been compiled'); |
There was a problem hiding this comment.
This is going to to cause the complete test suite to fail since several things run this test.
I'll try to come up with a better way to do this.
(Ab)use a static to mark the test as run once it has been run first time. If it's the first run and the test class is already defined then fail as the test's not valid.
|
OK, I think that clears up the test a bit. I'm not entirely happy with the static flag, but I can't find an easy way to get phpUnit to give me this behaviour out of the box. |
|
Is this a problem for every php version and by design of the php parser? Because I'd rather upgrade the PHP dependency to something where this is fixed. |
|
I've tested against 5.3.3, 5.4.9, and master, and the problem exists for all three, so I'd assume it exists in all versions. It's less noticeable since https://bugs.php.net/bug.php?id=55156 was fixed, as that fix causes namespace statements, which will often feature as the first thing in a PHP file, to clear the doc_comment compiler global. That doesn't help for files that don't use namespaces though. It also, obviously, doesn't affect classes that do have doc comments. As long as people ensure that all classes against which the annotation reader is expected to be run have a class level docblock comment, this isn't a problem. Sadly there's no way to automatically enforce that. |
|
As promised, I've raised the underlying problem as a bug with PHP: https://bugs.php.net/bug.php?id=64936 |
|
The underlying bug has now been fixed in PHP master, which means it's going to be quite some time before we start seeing the fix in real deployments. imo it's worth (and harmless) having the workaround in place until such a time that it's realistic to require a version of PHP with the real fix. |
Workaround PHP compiler globals storing doc_comments
The PHP parser stores the last docblock comment that it saw in a compiler global. When the compiler next sees something that might conceivably have a docblock comment, that thing becomes the owner of the comment, and the global is cleared.
Unfortunately the parser does not clear state between parses. A call to token_get_all() on something containing a docblock comment will cause the global to be set. The next file to be compiled by an include() will start off with that global, and this (under some circumstances) can cause the comment to be erroneously associated with another class.
This workaround ensures that after a call to token_get_all() we reset the parser to a state where the doc_comment compiler global contains an empty comment, and is therefore "safe" for us.
This is only a workaround: With this fix we have to hope that any other uses of token_get_all() in the user's codebase are also similarly protected. IMO this is a PHP bug (for which I shall raise a bug report), and should, long term, be fixed there.