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

arderyp · 2018-05-19T00:42:39Z

Q	A
Branch?	4.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27037
License	MIT
Doc PR	na

This is my first attempt at fixing #27037.

This approach works for me, and I believe it will work globally, but I am not entirely sure if it is the "right" way to do this. Its not that I think it's the "wrong" way to do it, its just that the phpunit-bridge code structure is not the type of code structure that I'm used to, and I'm still new to this project, so I don't know all of the ins and outs, or if there might be a more ideal place to inject the logic we want.

Also, I think my code may be a bit redundant, and I was hoping to get feedback on that. @nicolas-grekas (or anyone else), do you think it is safe to simply check if the deprecation message contains the substring service is private, getting it from the container...? if so, then I think we can simply get rid of my $isNoticeForContainerGetUsage bit altogether. If I am understanding things properly, $isNoticeForContainerGetUsage will always be true if $isPrivateServiceNotice is true, thus we can safely remove the former (as the later is more intuitive, IMO).

The last thing I wanted to confirm was my interpretation of the stack trace, as detailed in my code comment. If the bit after NOTE is not correct, this code might not be a proper solution. I tested various contexts where the error should be suppressed (from within tests) and also where it should not (from within controllers, etc), which is what I based my NOTE comment on.

nicolas-grekas

Lgtm thanks! Here are a few minor comments. The most important one is that we should do the same with the "has" method.

nicolas-grekas · 2018-05-19T09:00:06Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+            // fetching of private services within tests, so we no longer
+            // need to warn about this behavior:
+            //
+            // https://symfony.com/blog/new-in-symfony-4-1-simpler-service-testing


Should be removed, we usually don't link to the blog.

Easy enough

nicolas-grekas · 2018-05-19T09:00:45Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+            // 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 be an object
+            // instance of PHPUnit_Framework_TestCase.


Should use backslash instead of underscore

easy enough

nicolas-grekas · 2018-05-19T09:01:03Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+            // represents the action that called the deprecated code. In the
+            // scenario that we want to suppress, the 4th event be an object
+            // instance of PHPUnit_Framework_TestCase.
+            if (isset($trace[3]) && isset($trace[3]['object'])) {


The first isset is not needed

I didn't originally have it, because I've never ever sen a stack track (particularly one for a framework) less than 4 elements, but I added it "just to be safe". I will remove it.

You don't need it because isset both check that $trace[3] exists and then that $trace[3]['object'] exists: https://3v4l.org/4XGmL

ahh, thanks for the tip, should have known

nicolas-grekas · 2018-05-19T09:02:19Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+            // instance of PHPUnit_Framework_TestCase.
+            if (isset($trace[3]) && isset($trace[3]['object'])) {
+                $noticeSource = $trace[2];
+                $noticeSourceCaller = $trace[3]['object'];


I'd suggest to remove these two intermediary variables: the note just above is clear enough

easy enough

nicolas-grekas · 2018-05-19T09:03:58Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+            if (isset($trace[3]) && isset($trace[3]['object'])) {
+                $noticeSource = $trace[2];
+                $noticeSourceCaller = $trace[3]['object'];
+                $isPrivateServiceNotice = (false !== strpos($msg, ' service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.'));


This missing the message triggered by the "has" method, which should be handled here also. Maybe just check for the first common words? Should be enough since we also check the class + method triggering this just below.

good point, I can check for the common prefix of " service is private, instead. I will then also need to change $isNoticeForContainerGetUsage to $isNoticeForContainerGetHasUsage and also include coverage for the function equaling has

nicolas-grekas · 2018-05-19T09:04:32Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+                $noticeSource = $trace[2];
+                $noticeSourceCaller = $trace[3]['object'];
+                $isPrivateServiceNotice = (false !== strpos($msg, ' service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.'));
+                $isNoticeForContainerGetUsage = ('Symfony\Component\DependencyInjection\Container' === $noticeSource['class'] && 'get' === $noticeSource['function']);


Enclosing brackets should be removed (same above)

arderyp · 2018-05-19T18:05:42Z

@nicolas-grekas, I think the most recent commit above address all of your notes, but I'm happy to do another pass if need be. Thanks for the feedback.

…container in tests

nicolas-grekas · 2018-05-21T09:19:26Z

Thank you @arderyp.

@nicolas-grekas

…rvies from container in tests (arderyp) This PR was submitted for the master branch but it was squashed and merged into the 4.1 branch instead (closes #27312). Discussion ---------- Supress deprecation notices thrown when getting private servies from container in tests | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27037 | License | MIT | Doc PR | na This is my first attempt at fixing #27037. This approach works for me, and I believe it will work globally, but I am not entirely sure if it is the "right" way to do this. Its not that I think it's the "wrong" way to do it, its just that the `phpunit-bridge` code structure is not the type of code structure that I'm used to, and I'm still new to this project, so I don't know all of the ins and outs, or if there might be a more ideal place to inject the logic we want. Also, I think my code may be a bit redundant, and I was hoping to get feedback on that. @nicolas-grekas (or anyone else), do you think it is safe to simply check if the deprecation message contains the substring ` service is private, getting it from the container...`? if so, then I think we can simply get rid of my `$isNoticeForContainerGetUsage` bit altogether. If I am understanding things properly, `$isNoticeForContainerGetUsage` will always be true if `$isPrivateServiceNotice` is true, thus we can safely remove the former (as the later is more intuitive, IMO). The last thing I wanted to confirm was my interpretation of the stack trace, as detailed in my code comment. If the bit after `NOTE` is not correct, this code might not be a proper solution. I tested various contexts where the error should be suppressed (from within tests) and also where it should not (from within controllers, etc), which is what I based my `NOTE` comment on. Commits ------- 00603bf Supress deprecation notices thrown when getting private servies from container in tests

stof · 2018-05-21T12:28:19Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+            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;


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

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

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

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

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

see: #27342

xabbuh · 2018-05-23T08:45:23Z

Wouldn't this hide wrong usages of the container in tests on 3.4 which would then break when updating to 4.0?

stof · 2018-05-23T10:00:52Z

Based on the information provided in #27037, this PR should be reverted entirely, as it is hiding actual deprecated calls. the initial bug report was invalid.

…ivate servies from container in tests (arderyp)" This reverts commit 70c70e2, reversing changes made to 7497ad4.

* 4.1: (26 commits) Revert "bug #27312 Supress deprecation notices thrown when getting private servies from container in tests (arderyp)" [HttpKernel] reset kernel start time on reboot Add code of Conduct links in our README bumped Symfony version to 4.0.12 [FrameworkBundle] Fix using test.service_container when Client is rebooted [DI] never inline lazy services updated VERSION for 4.0.11 updated CHANGELOG for 4.0.11 bumped Symfony version to 3.4.12 updated VERSION for 3.4.11 updated CHANGELOG for 3.4.11 Default testsuite to latest PHPUnit 6.* [Github] Update the pull-request template bumped Symfony version to 2.8.42 updated VERSION for 2.8.41 updated CHANGELOG for 2.8.41 Tweak Argon2 test config [HttpFoundation] Fix cookie test with xdebug [FrameworkBundle] cleanup generated test container [Serializer] Check the value of enable_max_depth if defined ...

nicolas-grekas · 2018-05-25T14:57:10Z

Reverted in ab09fcc, let's keep talking about this in #27342

nicolas-grekas added this to the 4.1 milestone May 19, 2018

nicolas-grekas requested changes May 19, 2018

View reviewed changes

carsonbot added the Status: Needs Work label May 19, 2018

carsonbot removed the Status: Needs Work label May 19, 2018

nicolas-grekas approved these changes May 19, 2018

View reviewed changes

carsonbot added the Status: Reviewed label May 19, 2018

chalasr added PhpUnitBridge Bug labels May 20, 2018

nicolas-grekas changed the title ~~Supress deprecation notices thrown when getting private servies from container in tests~~ [PhpUnitBridge] Supress deprecation notices thrown when getting private services from container in tests May 21, 2018

Supress deprecation notices thrown when getting private servies from …

00603bf

…container in tests

nicolas-grekas changed the base branch from master to 4.1 May 21, 2018 09:19

nicolas-grekas merged commit 00603bf into symfony:4.1 May 21, 2018

stof reviewed May 21, 2018

View reviewed changes

fabpot mentioned this pull request May 21, 2018

Release v4.1.0-BETA2 #27331

Merged

arderyp mentioned this pull request May 22, 2018

[PhpUnitBridge] Making private service deprecation warning supressor more strict #27342

Closed

nicolas-grekas added a commit that referenced this pull request May 25, 2018

Revert "bug #27312 Supress deprecation notices thrown when getting pr…

ab09fcc

…ivate servies from container in tests (arderyp)" This reverts commit 70c70e2, reversing changes made to 7497ad4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arderyp commented May 19, 2018 •

edited by chalasr

Loading

nicolas-grekas left a comment

nicolas-grekas May 19, 2018

arderyp May 19, 2018 •

edited

Loading

nicolas-grekas May 19, 2018

arderyp May 19, 2018

nicolas-grekas May 19, 2018 •

edited

Loading

arderyp May 19, 2018

jvasseur May 19, 2018

arderyp May 19, 2018

nicolas-grekas May 19, 2018

arderyp May 19, 2018

nicolas-grekas May 19, 2018

arderyp May 19, 2018

nicolas-grekas May 19, 2018

arderyp May 19, 2018

arderyp commented May 19, 2018

nicolas-grekas commented May 21, 2018

stof May 21, 2018

nicolas-grekas May 21, 2018

arderyp May 21, 2018

nicolas-grekas May 21, 2018

arderyp May 21, 2018

arderyp May 22, 2018

xabbuh commented May 23, 2018

stof commented May 23, 2018

nicolas-grekas commented May 25, 2018

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

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

Conversation

arderyp commented May 19, 2018 • edited by chalasr Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arderyp May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arderyp commented May 19, 2018

nicolas-grekas commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented May 23, 2018

stof commented May 23, 2018

nicolas-grekas commented May 25, 2018

arderyp commented May 19, 2018 •

edited by chalasr

Loading

arderyp May 19, 2018 •

edited

Loading

nicolas-grekas May 19, 2018 •

edited

Loading