Problem/Motivation

#2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code added

* @expectedDeprecationMessage Use of the /migration_templates directory to store migration configuration files is deprecated in Drupal 8.1.0 and will be removed before Drupal 9.0.0.

… but @expectedDeprecationMessage is not supported, only @expectedDeprecation is.

Proposed resolution

TBD.

Remaining tasks

Step 1: figure out why this still passes tests.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 2928645-5.patch1.03 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

alexpott’s picture

I think you might find that the annotation parser used is quite loose so it just finds @expectedDeprecation and it works just fine...

alexpott’s picture

Hmmm nope - it's just ignoring the expectedDeprecation completely. It needs the @legacy but there's other stuff not happening when @expectedDeprecation is stuff. My guess is that this is a bug with Symfony and how the deprecation stuff works when process isolation is on as it is in KernelTestBase.

alexpott’s picture

Well actually if you fix it to use @expectedDeprecation then it works as expected - the test currently fails cause the message has been updated to include the change record but the question is why is the @group legacy enough...

alexpott’s picture

Status: Active » Needs review
FileSize
1.03 KB

Here's a patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It is a bit sad that it cannot use an explicit API like PHPUnit does now. Otherwise this would have been detected already.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +@deprecated

It is a bit sad that it cannot use an explicit API like PHPUnit does now. Otherwise this would have been detected already.

Yep, setExpectedDeprecation() would've been better.

but the question is why is the @group legacy enough...

I think we need to understand this first before we commit this?

dawehner’s picture

Yep, setExpectedDeprecation() would've been better.

Well, I'm quite sure that there is a reason: \Symfony\Bridge\PhpUnit\DeprecationErrorHandler::register needs to run before the actual test is executed, which is the place, where you can just parse annotations and not look at the actual code.

Wim Leers’s picture

I guess what we really want is for PHPUnit to have explicit support for this, rather than it being something added by Symfony?

alexpott’s picture

@Wim Leers I guess that might be nice. Considering phpunit mock objects now do the @trigger_error() thing there might be scope to add this to PHPUnit. See https://github.com/sebastianbergmann/phpunit-mock-objects/issues/271

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Needs review » Reviewed & tested by the community

The test passes because it's annotated @group legacy, and then the annotation is ignored. That's why it says:

Legacy deprecation notices (6)

After the patch you don't get this message, and you just get a passing test.

Therefore, RTBC.

Moving to 8.7.x because it's a test improvement.

catch’s picture

Version: 8.7.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 79cb8c4 and pushed to 8.8.x. Thanks!

  • catch committed 79cb8c4 on 8.8.x
    Issue #2928645 by alexpott, Wim Leers, dawehner, Mile23: \Drupal\Tests\...

Status: Fixed » Closed (fixed)

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