Fix the conditional definition of the SymfonyTestsListener #23145
Conversation
|
Should we do the same for Command and TestRunner? |
|
These classes are used by the |
|
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. |
|
@jpauli but an early return in the global code of the file placed before the class definition should make it conditional. |
|
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/a 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-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
|
@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. |
|
yep, strange... |
|
@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) |
|
You have a return in an if(). So it needs execution. Classes will get parsed before. Doing that : still declares class Foo ... |
|
OK, then explain me why the early return works fine in some 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. |
|
@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 ? |
|
Yep, same to me. We already made this kind of changes (e.g. when moving to ChildDefinition if I recall well) |
|
@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. |
|
@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.