Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix the conditional definition of the SymfonyTestsListener#23145
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJun 12, 2017
Should we do the same for Command and TestRunner? |
stof commentedJun 12, 2017
These classes are used by the |
jpauli commentedJun 12, 2017
Class definitions are fully resolved at compile time and require zero runtime (when they are not conditional), so yes, they gets parsed before code gets executed. |
stof commentedJun 12, 2017
@jpauli but an early return in the global code of the file placed before the class definition should make it conditional. |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 (failure unrelated)
fabpot commentedJun 12, 2017
Thank you@stof. |
… (stof)This PR was merged into the 3.3 branch.Discussion----------Fix the conditional definition of the SymfonyTestsListener| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aThis is a continuation of the attempts at fixing the PHPUnit 5 compatibility layer for the listener.The signature mismatch error still happened when using the PHPUnit PHAR instead of a source install (hint: people using `simple-phpunit` are using a source install).It looks like the class definition gets loaded by PHP before executing the code placed above it (and so the early return breaks). Putting the code inside a `else` instead works fine (the class definition probably cannot bubble up).The known difference between the PHAR and a source install is that the source install relies on autoloading while the PHAR loads all PHPUnit classes through `require_once` eagerly (and so the parent class already exists when using the Symfony file).@jpauli is it an expected behavior that early returns before class definitions don't work consistently ?Regarding the patch itself, an alternative would be to move the PHPUnit 6+ implementation to a dedicated class instead, and use a `class_alias` for the else clause too. But I don't think it is worth it.Commits-------0ec8b1c Fix the conditional definition of the SymfonyTestsListener
stof commentedJun 12, 2017
@nicolas-grekas the collapsing seems to behave weird for this failure btw. The folding closing code matches the wrong opening code, hiding the failing code. |
nicolas-grekas commentedJun 12, 2017
yep, strange... |
stof commentedJun 12, 2017
@nicolas-grekas may be related to the fact that the only difference in the collapse key is the number at the end (just trying to guess) |
jpauli commentedJun 12, 2017
You have a return in an if(). So it needs execution. Classes will get parsed before. Doing that : still declares class Foo ... |
stof commentedJun 12, 2017
OK, then explain me why the early return works fine insome cases but not in others. Your comment tells me that I should have faced the same issue all the time when using PHUnit 5, not only when using the PHAR. It really looks like it depends whether the parent class is already loaded before loading this file or no. |
stof commentedJun 12, 2017
@nicolas-grekas The comment given by@jpauli makes me think we should probably never use global early returns anywhere when wanting to conditionally define classes (be it in symfony itself or in the polyfill repo). What do you think ? |
nicolas-grekas commentedJun 12, 2017
Yep, same to me. We already made this kind of changes (e.g. when moving to ChildDefinition if I recall well) |
jpauli commentedJun 12, 2017
@stof I dont know. I explain you how things should work. If you experience a strange behavior, I suggest you try to debug them or provide simple tiny reproducers if you think you found bugs. |
stof commentedJun 12, 2017
@jpauli what I don in the |
This is a continuation of the attempts at fixing the PHPUnit 5 compatibility layer for the listener.
The signature mismatch error still happened when using the PHPUnit PHAR instead of a source install (hint: people using
simple-phpunitare using a source install).It looks like the class definition gets loaded by PHP before executing the code placed above it (and so the early return breaks). Putting the code inside a
elseinstead works fine (the class definition probably cannot bubble up).The known difference between the PHAR and a source install is that the source install relies on autoloading while the PHAR loads all PHPUnit classes through
require_onceeagerly (and so the parent class already exists when using the Symfony file).@jpauli is it an expected behavior that early returns before class definitions don't work consistently ?
Regarding the patch itself, an alternative would be to move the PHPUnit 6+ implementation to a dedicated class instead, and use a
class_aliasfor the else clause too. But I don't think it is worth it.