Return type deprecations for library tests

[derrabus]

Symfony version(s) affected: 4.4-dev

One of my open source projects starts to produce failing travis builds because of the deprecation notices triggered by DebugClassLoader since #30323. In order to make DebugClassLoader helpful for libraries, I think we need to fine tune it a bit:

• Almost all of the notices triggered here affect vendor code (here: Pimple and Silex). Four deprecation notices are flagged as "direct deprecation notices" by PHPUnit Bridge, but only two of the notices are actually triggered by my own classes.
• I'm pretty sure that Silex and Pimple won't be patched, so I need a way to silence deprecation triggered by vendor code.
• The branch that is tested here maintains php 5 compatibility. Because of that, I'd probably want to deactivate return type checks completely?

For reference, the errors from my travis log:

Remaining direct deprecation notices (4)

  1x: Method "Symfony\Component\HttpKernel\HttpKernelInterface::handle()" will return "Response" as of its next major version. Doing the same in child class "Silex\Application" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Psr\Container\ContainerInterface::has()" will return "bool" as of its next major version. Doing the same in child class "Pimple\Psr11\Container" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface::supports()" will return "bool" as of its next major version. Doing the same in child class "Rabus\Psr11ServiceProvider\ArgumentResolver" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface::resolve()" will return "iterable" as of its next major version. Doing the same in child class "Rabus\Psr11ServiceProvider\ArgumentResolver" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

Remaining indirect deprecation notices (8)

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" will return "array" as of its next major version. Doing the same in child class "Silex\EventListener\MiddlewareListener" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" will return "array" as of its next major version. Doing the same in child class "Silex\EventListener\ConverterListener" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" will return "array" as of its next major version. Doing the same in child class "Silex\EventListener\StringToResponseListener" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\Routing\Matcher\RequestMatcherInterface::matchRequest()" will return "array" as of its next major version. Doing the same in child class "Silex\Provider\Routing\LazyRequestMatcher" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" will return "array" as of its next major version. Doing the same in child class "Silex\ExceptionHandler" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface::supports()" will return "bool" as of its next major version. Doing the same in child class "Silex\AppArgumentValueResolver" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface::resolve()" will return "iterable" as of its next major version. Doing the same in child class "Silex\AppArgumentValueResolver" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testServiceRegistration from Rabus\Psr11ServiceProvider

  1x: Method "Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface::redirect()" will return "array" as of its next major version. Doing the same in child class "Silex\Provider\Routing\RedirectableUrlMatcher" will be required when upgrading.

    1x in Psr11ServiceProviderTest::testControllerArgument from Rabus\Psr11ServiceProvider

[nicolas-grekas]

Almost all of the notices triggered here affect vendor code (here: Pimple and Silex). Four deprecation notices are flagged as "direct deprecation notices" by PHPUnit Bridge, but only two of the notices are actually triggered by my own classes.

Someone would need to have a look for a way to improve this, if possible

I'm pretty sure that Silex and Pimple won't be patched, so I need a way to silence deprecation triggered by vendor code.

see #33236

The branch that is tested here maintains php 5 compatibility. Because of that, I'd probably want to deactivate return type checks completely?

That's the use case why adding @return annotations in your codebase silences the deprecations. That's how you should do it.

[derrabus]

The branch that is tested here maintains php 5 compatibility. Because of that, I'd probably want to deactivate return type checks completely?

That's the use case why adding @return annotations in your codebase silences the deprecations. That's how you should do it.

I was merely thinking about some sort of global switch for PHPUnit bridge.

I mean, in this particular case we're talking about a tiny codebase, where I certainly could add those annotations. But in practice, we would tell library vendors to add those @return annotations everywhere. And as soon as they bump to php 7.2 they would need to remove all those annotations again so that they get proper deprecations. That sounds like a pretty complex workflow for something that have been an env var setting.

[mdlutz24]

Just as a talking point this shows a run of the Drupal Test suite with the new classloader. There's a couple unrelated fails there as well, but the bulk of the failures are the new deprecations from the merge. In general, I think this is fine. We are able to suppress specific deprecations in our test suite and address them in follow ups. A couple of interesting ones here..

1x: Method "Symfony\Component\DependencyInjection\ContainerInterface::get()" will return "?object" as of its next major version. Doing the same in child class "Drupal\Component\DependencyInjection\Container" will be required when upgrading.
1x in FieldConfigAccessControlHandlerTest::setUp from Drupal\Tests\field\Unit

This test is running against php 7.1, so I can't return typehint ?object yet, but when we open the D9 branch, we will be on php 7.2 or 7.3, so I don't think it bothers us necessarily. It's tempting to consider not throwing that deprecation on php 7.1, but at the end of the day, SF5 will require php 7.2 and a object typehint, so the deprecation is valid regardless.

1x: Method "SebastianBergmann\Comparator\Comparator::accepts()" will return "bool" as of its next major version. Doing the same in child class "Prophecy\Comparator\ClosureComparator" will be required when upgrading.
1x in ManyPlaceholderTest::testManyNoJsPlaceHolders from Drupal\Tests\big_pipe\Unit\Render

1x: Method "SebastianBergmann\Comparator\ObjectComparator::accepts()" will return "bool" as of its next major version. Doing the same in child class "Prophecy\Comparator\ProphecyComparator" will be required when upgrading.
1x in ManyPlaceholderTest::testManyNoJsPlaceHolders from Drupal\Tests\big_pipe\Unit\Render

Definitely some issues in vendor libraries that we will have to figure out when we open our Drupal 9 branch too.

I think at the end of the day we will end up needing to drop the {@inheritdoc} and copy the full docblock to eliminate the deprecation notice for Drupal 9 core but pass it through to our contrib projects if any of them happen to be extending or implementing any Symfony classes or interfaces, then we can add the actual php return typehints in Drupal 10 which would likely release on Symfony 5.4 in a couple years.

[mdlutz24]

Now what I would really be interested in (and this might either be a separate Symfony issue, or maybe something we do in Drupal core in a extended Debug classloader) is a way to use this ourselves, trigger deprecations notices on Drupal interfaces that aren't return typehinted downstream. We've been trying to find a way to introduce return type hints in a backwards compatible way, and I really think the work here (and other work in the Debug ClassLoader in SF4) has been really great work, and solves a lot of problems we've been having trying to trigger deprecation errors in situations where we can't easily do it in code.

[nicolas-grekas]

Follow up in #33283