Skip to content

[PhpUnitBridge] Supress deprecation notices thrown when getting private services from container in tests #27312

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 21, 2018
Merged

[PhpUnitBridge] Supress deprecation notices thrown when getting private services from container in tests #27312

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,30 @@ public static function register($mode = 0)
}

$trace = debug_backtrace(true);

// Silence deprecation warnings about private service accessed
// from the service container if done so from a Test class.
// As of Symfony 4.1, there is a new TestContainer that allows
// fetching of private services within tests, so we no longer
// need to warn about this behavior.
//
// NOTE: the event at the top of the stack $trace (index 0) should
// always be the PhpUnitBridge's DeprecationErrorHandler; the
// second event (index 1) should be the trigger_error() event;
// the third event (index 2) should be the actual source of the
// triggered deprecation notice; and the fourth event (index 3)
// represents the action that called the deprecated code. In the
// scenario that we want to suppress, the 4th event will be an
// object instance of \PHPUnit\Framework\TestCase.
if (isset($trace[3]['object'])) {
$isPrivateServiceNotice = false !== strpos($msg, ' service is private, ');
$isNoticeForContainerGetHasUsage = 'Symfony\Component\DependencyInjection\Container' === $trace[2]['class'] && in_array($trace[2]['function'], array('get', 'has'));
$noticeWasTriggeredByPhpUnitTest = $trace[3]['object'] instanceof \PHPUnit\Framework\TestCase;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this test for the KernelTestCase instead (the parent TestCase does not have the client) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instanceof does it, isn't it? Not sure we need to be more strict, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof and @nicolas-grekas I didn't want to be any more strict than \PHPUnit\Framework\TestCase because I for one have unit tests that extend this directly instead of KernelTestCase. I'm not sure if I am accessing the service container on any of those, I'd have to check, but I doubt it. But, this might be a good point. Is the new TestContainer explicitly tied to KernelTestCase? If so, then the deprecation warning probably shouldn't be suppressed for raw extensions of \PHPUnit\Framework\TestCase, as the TestContainer only provides access to private services via extensions of KernelTestCase, meaning attempts to do so via TestCase extensions should throw errors/warnings.

If that is not the case, then I think making this comparison as general as possible by checking for extensions of TestCase is fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check this question and make the PR if needed? The reasoning lgtm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas I will take a look when I get a chance tomorrow and report back

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see: #27342

if ($isPrivateServiceNotice && $isNoticeForContainerGetHasUsage && $noticeWasTriggeredByPhpUnitTest) {
return false;
}
}

$group = 'other';
$isVendor = DeprecationErrorHandler::MODE_WEAK_VENDORS === $mode && $inVendors($file);

Expand Down