-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Allow Drupal to wrap the Symfony test listener #37708
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
Allow Drupal to wrap the Symfony test listener #37708
Conversation
Here's the related Drupal issue - https://www.drupal.org/project/drupal/issues/3162403 - that fixes this the very ugly way. |
@@ -61,7 +61,7 @@ public function __construct($message, array $trace, $file) | |||
$line = $trace[$i]; | |||
$this->triggeringFile = $file; | |||
if (isset($line['object']) || isset($line['class'])) { | |||
if (isset($line['class']) && 0 === strpos($line['class'], SymfonyTestsListenerFor::class)) { | |||
if (isset($line['class']) && is_subclass_of($line['class'], TestListener::class)) { |
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.
this would now also apply for other PHPUnit TestListener implementations which have nothing to do with the Symfony deprecation helper. That's risky to me.
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.
Good point. Gone for a new approach that doesn't check the class at all.
Btw, I suggest you to open an issue (separate from this PR) describing the use cases of Drupal that require you to wrap the symfony deprecation listener. Some of them might be suitable to be integrated in the symfony/phpunit-bridge directly, which would simplify your code in the future (if you don't need to mess with the internals of the bridge implementation anymore). |
@stof thanks for the review. How about changing tack? - and try unserialising the message - if that works use the information. That way we're only entering this code path when dealing with a serialized message - and we no longer care so much about the stack trace. See the latest commits for this approach. I've been meaning to create an issue and PR for the additions Drupal is doing. I did that with the expectedDeprecation work - took a while to land - but now it has that's what's prompted Drupal to move to the SF5 version of the bridge and do less awkward stuff - but now we've run into this. |
f563cf5
to
91de46d
Compare
Thank you @alexpott. |
…rt) (fabpot, alexpott) This PR was merged into the 5.1 branch. Discussion ---------- Allow Drupal to wrap the Symfony test listener (5.1 backport) | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | kinda | New feature? | no | Deprecations? | no | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> This is a backport of #37708 Commits ------- 244e8d2 Revert "Swallow errors" ee8cc26 Swallow errors f9bfe7f Allow Drupal to wrap the Symfony test listener
Drupal has a test listener that wraps the Symfony listener as we need to manipulate deprecation testing to skip specific deprecations. We've managed to remove some how custom stuff and reaching into Symfony's privates via reflection recently (and fix a bug or two) but now we're getting output like:
instead of
We can fix this by aliasing and copying the \Symfony\Bridge\PhpUnit\DeprecationErrorHandler\Deprecation class but ideally that class could be a bit more liberal in its implementation.