Skip to content
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

[PHPUnitBridge] Mute deprecations triggered from phpunit #32443

Open
merged 1 commit into
base: 4.3
from Jul 18, 2019

Conversation

Projects
None yet
4 participants
@greg0ire
Copy link
Contributor

commented Jul 8, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Request by @nicolas-grekas here: #32289 (comment)

@greg0ire greg0ire force-pushed the greg0ire:muted-deprecations branch from 430589e to 780579f Jul 8, 2019

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@phansys, please review, and maybe test?

@phansys

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@phansys, please review, and maybe test?

See https://travis-ci.org/symfony/symfony/jobs/556567606.

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Thanks for this, looks like it does not work yet. I will try to make your PoC run locally and debug this :)

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

status: needs work

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Ah wait… the way to properly test this would be do add rm -fr vendor/symfony/phpunit-bridge -fr && cp -r src/Symfony/Bridge/PhpUnit/ vendor/symfony/phpunit-bridge somewhere in the build script

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

I created my own PoC, but it does not work either… will have to investigate https://travis-ci.org/symfony/symfony/jobs/556660318

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jul 10, 2019

@greg0ire greg0ire force-pushed the greg0ire:muted-deprecations branch 2 times, most recently from b7a96eb to a686300 Jul 10, 2019

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Ok so I have fixed most occurences of the deprecation. Some remain because of

throw new RuntimeException(sprintf('Invalid handler service "%s": type-hint of argument "$%s" in method "%s::__invoke()" must be a class , "%s" given.', $serviceId, $parameters[0]->getName(), $handlerClass->getName(), $type));

or because of the deprecation serialization system (I have to work on that one, I can probably reuse the originatingClass() method).

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

because of the deprecation serialization system (I have to work on that one, I can probably reuse the originatingClass() method).

Actually, I don't think I should attempt to fix that one: I don't have enough information to achieve it in a robust manner: a:4:{s:11:"deprecation";s:51:"Function ReflectionType::__toString() is deprecated";s:5:"class";s:41:"Symfony\Bridge\Twig\Tests\AppVariableTest";s:6:"method";s:14:"testGetSession";s:15:"triggering_file";s:110:"/home/travis/build/greg0ire/symfony/.phpunit/phpunit-6.5/vendor/phpunit/phpunit-mock-objects/src/Generator.php";}

The interesting part here is trigerring_file, but detecting vendor/phpunit looks fragile. Should I go for it anyway? Or should I just give up in that particular case? Please advise @nicolas-grekas

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

There are also a bunch of deprecations that don't seem to go through the bridge at all in the Debug component, I don't think I can do much about that…

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Checking for /vendor/phpunit/ (using DIRECTORY_SEPARATOR) LGTM. That's a pragmatic choice for insulated tests.

@greg0ire greg0ire force-pushed the greg0ire:muted-deprecations branch 2 times, most recently from b28be9f to 69ba9d0 Jul 15, 2019

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Done. Here is what it looks like on my PoC: https://travis-ci.org/greg0ire/symfony/jobs/559138971
The only occurences left are

  • when the bridge is not in use
  • when Symfony (in that case Messenger) is at fault.
@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Status: needs review

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

I met with Nicolas, and we established that I need to take care of the deprecations not going through the bridge error handler in the Debug component. We also established I need to make a separate PR to backport this to 3.4, where the code for the bridge is very different.

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Status: needs work

@greg0ire greg0ire force-pushed the greg0ire:muted-deprecations branch from 469d8be to 9ae8ffe Jul 16, 2019

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Ok now we're only left with the Messenger deprecation, which should probably be fixed in another PR: https://travis-ci.org/greg0ire/symfony/jobs/559682090

Status: needs review

@nicolas-grekas
Copy link
Member

left a comment

thanks, let's fix the Messenger deprecation in the same PR, enough bureaucracy :)

@greg0ire greg0ire force-pushed the greg0ire:muted-deprecations branch 2 times, most recently from 54df46c to 8724a39 Jul 17, 2019

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

thanks, let's fix the Messenger deprecation in the same PR, enough bureaucracy :)

Ok! Done. Here is a proof of that claim: https://travis-ci.org/greg0ire/symfony/jobs/560211283

Please review again :)

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

We also established I need to make a separate PR to backport this to 3.4, where the code for the bridge is very different.

Nicolas has realized that I actually don't need to do that PR since 3.4 is tested with the version 4 of the bridge, which will contain the fix.

A separate PR has been done regarding the Debug deprecations: #32592

As for the Messenger deprecation, the fix should stay in this PR since there is no Messenger on 3.4.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Can you please rebase?

@greg0ire greg0ire force-pushed the greg0ire:muted-deprecations branch from 8724a39 to a4ab13d Jul 18, 2019

@greg0ire

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Done

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Thank you @greg0ire.

@nicolas-grekas nicolas-grekas merged commit a4ab13d into symfony:4.3 Jul 18, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jul 18, 2019

bug #32443 [PHPUnitBridge] Mute deprecations triggered from phpunit (…
…greg0ire)

This PR was merged into the 4.3 branch.

Discussion
----------

[PHPUnitBridge] Mute deprecations triggered from phpunit

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Request by @nicolas-grekas here: #32289 (comment)

Commits
-------

a4ab13d Mute deprecations triggered from phpunit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.