Problem/Motivation

Deprecation messages are not being unserialised since #3156998: Using @requires extension_name in PHPUnit unit tests fails if extension is not present.

See https://www.drupal.org/pift-ci-job/1774572

Steps to reproduce

Proposed resolution

This can only be properly fixed upstream in Symfony because of how it decides to unserialise deprecation information in \Symfony\Bridge\PhpUnit\DeprecationErrorHandler\Deprecation

It's looking for a class starting with Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerFor after it has ignored all the PHPUnit classes. We need to change:

            if (isset($line['class']) && 0 === strpos($line['class'], SymfonyTestsListenerFor::class)) {

to 

      if (isset($line['class']) && is_subclass_of($line['class'], TestListener::class)) {

Remaining tasks

Decide whether we'll use this fix or wait for upstream - https://github.com/symfony/symfony/pull/37708 https://github.com/symfony/symfony/pull/38031

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#14 3162403-2-14.patch2.31 KBalexpott
#3 3162403-3.patch11.59 KBalexpott
#3 2-3-interdiff.txt872 bytesalexpott
#2 3162403-2.patch11.57 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs manual testing
Related issues: +#3156998: Using @requires extension_name in PHPUnit unit tests fails if extension is not present
FileSize
3162403-2.patch11.57 KB

As we don't support phpt testing we can't write a test for this. So it needs manual testing. At best DrupalCI can confirm that we've not broken the positive deprecation tests with this approach.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2-3-interdiff.txt872 bytes
3162403-3.patch11.59 KB

Keeping aligned with what's happening upstream.

Tested locally by applying https://www.drupal.org/files/issues/2020-07-29/3160405-11.patch and running core/modules/comment/tests/src/Functional/CommentAnonymousTest.php... the new approach works just great - you see

Remaining self deprecation notices (2)

  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

As opposed to serialised guff.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Looks like upstream is planning on merging the PR for next release (was tagged), so I suggest to wait, the bug is noisy printout but functionally all is good.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Upstream https://github.com/symfony/symfony/pull/37708 was merged on Aug 23, 2020, so Symfony 5.1.4 should have it.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Postponed

Filed #3168107: Bump composer.json symfony/phpunit-bridge constraint to ^5.1.4.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Postponed » Active
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review

Queued to test on top of commited

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Unfortunately the PR only made it into the master branch and not the 5.1 branch of Symfony so we still have an issue. Once that PR lands in a SF release it should automatically fix this issue - so that this issue can become about fixing the composer.json to a version the works 100% correctly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes

https://github.com/symfony/symfony/pull/38031 exists to merge the change to 5.1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

So now we're waiting on the next SF5 release as the upstream fix has now been merged to the SF5 branch.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: Fix deprecation error message output » Bump composer.json symfony/phpunit-bridge constraint to ^5.1.6 to fix badly formed deprecation error message
Status: Needs review » Postponed

Thanks @alexpott - upstream merged into 5.x, but it will probably be a month's time before 5.1.6 will be out. Postponing in the meantime.

Edit - xpost with #11

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Postponed » Active

5.1.6 released today.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3162403-2-14.patch2.31 KB

Here's the patch to update symfony phpunit - I didn't bump the constraint because whilst the upgrade fixes a bug I don't think bumping is warranted.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: Bump composer.json symfony/phpunit-bridge constraint to ^5.1.6 to fix badly formed deprecation error message » Upgrade symfony/phpunit-bridge to 5.1.6 to fix badly formed deprecation error message
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

#14 agreed. Tested in a TravisCI build with a contrib module that throws deprecations, https://travis-ci.org/github/mondrake/d8-unit/builds/731228328, OK.

RTBC

  • catch committed 0774f68 on 9.1.x 
    Issue #3162403 by alexpott, mondrake: Upgrade symfony/phpunit-bridge to...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 0774f68 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +9.1.0 release notes