Skip to content

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

Conversation

alexpott
Copy link
Contributor

Q A
Branch? 5.1
Bug fix? kinda
New feature? no
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

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:

  2x: a:4:{s:11:"deprecation";s:191:"Calling Drupal\Tests\WebAssert::responseNotMatches with more than one argument is deprecated in drupal:9.1.0 and will throw an exception in drupal:10.0.0. See https://www.drupal.org/node/TODO";s:5:"class";s:52:"Drupal\Tests\comment\Functional\CommentAnonymousTest";s:6:"method";s:13:"testAnonymous";s:15:"triggering_file";s:74:"/Users/alex/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/WebAssert.php";}
    2x in DrupalListener::endTest from Drupal\Tests\Listeners

instead of

  2x: Calling Drupal\Tests\WebAssert::responseNotMatches with more than one argument is deprecated in drupal:9.1.0 and will throw an exception in drupal:10.0.0. See https://www.drupal.org/node/TODO
    2x in CommentAnonymousTest::testAnonymous from Drupal\Tests\comment\Functional

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.

@alexpott
Copy link
Contributor Author

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)) {
Copy link
Member

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.

Copy link
Contributor Author

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.

@stof
Copy link
Member

stof commented Jul 30, 2020

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).

@alexpott
Copy link
Contributor Author

@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.

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 30, 2020
@alexpott
Copy link
Contributor Author

@stof I created the issue you asked for - #37715

@fabpot fabpot force-pushed the make-unserialisation-of-deprecations-more-liberal branch from f563cf5 to 91de46d Compare August 23, 2020 17:24
@fabpot
Copy link
Member

fabpot commented Aug 23, 2020

Thank you @alexpott.

@fabpot fabpot merged commit 704c648 into symfony:master Aug 23, 2020
fabpot added a commit that referenced this pull request Sep 2, 2020
…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
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants