[Symfony 5] Symfony's DebugClassLoader triggers deprecation messages for missing return type hints, where there is no deprecation

Should this be resolved upstream if Symfony is issuing deprecations for third party code, when Drupal is not involved at all?

Does it mean PSR-7 as in the FIG standard, or Guzzle's psr7 component which is weirdly its PSR-17 implementation in the 2.0 branch? See #3220220: Update guzzlehttp/psr7 to 2.1.0.

There are return typehints in (e.g.) https://github.com/guzzle/psr7/blob/master/src/Request.php.

I think we might need to set SYMFONY_DEPRECATIONS_HELPER to something other than the default. Here's the docs for the env var.

     * The mode is a query string with options:
     *  - "disabled" to enable/disable the deprecation handler
     *  - "verbose" to enable/disable displaying the deprecation report
     *  - "quiet" to disable displaying the deprecation report only for some groups (i.e. quiet[]=other)
     *  - "max" to configure the number of deprecations to allow before exiting with a non-zero
     *    status code; it's an array with keys "total", "self", "direct" and "indirect"
     *
     * The default mode is "max[total]=0&verbose=1".

See https://symfony.com/doc/current/components/phpunit_bridge.html#internal-...

Apparently we can set SYMFONY_PATCH_TYPE_DECLARATIONS=deprecations=0

Looking at the docs:

 * It can also patch classes to turn docblocks into actual return types.
 * This behavior is controlled by the SYMFONY_PATCH_TYPE_DECLARATIONS env var,
 * which is a url-encoded array with the follow parameters:
 *  - "force": any value enables deprecation notices - can be any of:
 *      - "phpdoc" to patch only docblock annotations
 *      - "2" to add all possible return types
 *      - "1" to add return types but only to tests/final/internal/private methods
 *  - "php": the target version of PHP - e.g. "7.1" doesn't generate "object" types
 *  - "deprecations": "1" to trigger a deprecation notice when a child class misses a
 *                    return type while the parent declares an "@return" annotation

So that'll stop deprecations from being triggered and we can leave SYMFONY_DEPRECATIONS_HELPER alone.

#3231693: [Symfony 6] Add type hints to Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader::getClassAnnotations(), ::getMethodAnnotations(), ::getPropertyAnnotations(), ::getClassAnnotation(), ::getMethodAnnotation() and ::getPropertyAnnotation() is another case where DebugClassLoader is reporting issues in third-party code (Doctrine) that it has no jurisdiction over.

Even with the workaround in #9, this does seem worth reporting upstream as a weird and buggy default behavior.

I reached out to the Symfony community to discuss this issue. Here's the conversation:

alexpott Yesterday at 11:28 AM
@nicolasgrekas we’ve been working on identifying things we need to fix in Drupal for SF6 compatibility and we’ve run into new return type deprecations being triggered by the awesome new work you’ve done on the DebugClassloader - see https://www.drupal.org/project/drupal/issues/3232131#comment-14216127. Is there something we’re doing wrong? Maybe we’ve not got SYMFONY_DEPRECATIONS_HELPER set right but afaics all the return type deprecations are triggered from here https://github.com/symfony/symfony/blob/6.0/src/Symfony/Component/ErrorH... - so if we want to trigger them for our own code but not for vendor it’ll be tricky.

wouterj 23 hours ago
We're about to document this functionality. (see https://github.com/symfony/symfony-docs/pull/15802 )
The script is meant to trigger deprecations for any class: Let's push the community towards better typed systems.
We will also provide a utility to automatically add the correct return types to the project's methods (will be part of 5.4-dev within a couple days). This makes removing the deprecations effortless.
:tada:

wouterj 23 hours ago
Looking at the error, a slightly outdated 5.4-dev is used. The new error message is something along the lines of:
Method "Psr\Http\Message\StreamInterface::getMetadata()" might add "mixed" as a native return type declaration in the future. Do the same in child class "GuzzleHttp\Psr7\Stream" now to avoid errors or add an explicit @return annotation to suppress this message.

wouterj 23 hours ago
Sorry, last message (that I removed) was wrong. It should indeed trigger for Guzzle as well (but it is triggered as an "indirect" deprecation - meaning it's not a deprecation in your project).
Again, let's push the community towards better typing :slightly_smiling_face:

wouterj 23 hours ago
See https://symfony.com/doc/current/components/phpunit_bridge#direct-and-ind... for a bit more information about direct and indirect deprecations

alexpott 23 hours ago
@wouterj thanks for looking at this. I’m looking forward to the docs. We’ve got by with the default value for SYMFONY_DEPRECATIONS_HELPER since we added deprecation testing. When we’ve hit an indirect deprecation in our dependencies we’ve worked with the dependency to address it. The problem with the return type ones is that’ll be just too many to take that approach. (edited)

alexpott 23 hours ago
I think these return type deprecations are a different class than a real deprecation. There’s quite a bit of assumption that there will be a version of https://github.com/php-fig/http-message/blob/master/src/StreamInterface.php that adds real return types. Also mixing them with real deprecations makes it harder to focus on stuff that you know will definitely happen.

wouterj 23 hours ago
Hmm, I see your point. But at the same time, it'll be required to allow any OSS package to move towards typing. Adding a return type is a hard BC break (as it immediately forces your users to add them as well).
This deprecation is warning you that you might experience this hard BC break at any moment. You can avoid it by adding the correct return type now, or you can explicitly say "I know what I'm doing" by adding the @phpdoc.
The patch-type-declarations script that will be introduced is able to automatically do both actions for you. So in the end, it's a matter of making the decision and running the script.

wouterj 23 hours ago
There’s quite a bit of assumption that there will be a version of https://github.com/php-fig/http-message/blob/master/src/StreamInterface.php that adds real return types
Which isn't that unreasonable, given https://github.com/php-fig/log/compare/2.0.0...3.0.0 which caused Symfony to not support the latest Log PSR anymore :wink:
And that's all this deprecation is doing: warning you that this might happen, and letting you know that you can take action now to avoid that. (edited)

wouterj 23 hours ago
Anyway, I'm happy to hear opinions of others here :slightly_smiling_face:

alexpott 22 hours ago
I’m not sure - looking at the code in DebugClassloader - that it’ll make patches for vendor code. (which seems reasonable)

nicolasgrekas 22 hours ago
it will :slightly_smiling_face:

wouterj 21 hours ago
It'll trigger deprecations, but it won't patch the code (which is reasonable, as it's vendor code).
Package maintainers can use the tool on their packages to fix the deprecations.
:+1:

wouterj 21 hours ago
Alternatively, you can use SYMFONY_PATCH_TYPE_DECLARATIONS=deprecations=0. But let's focus on adding types (and explicit opt-outs) in the PHP community :slightly_smiling_face:

wouterj 13 hours ago
I see you know recommend the latter. That's really the unsafe way.
Doing SYMFONY_DEPRECATIONS_HELPER='max[direct]=0&max[total]= is much much much safer.
Alternatively, you can also create a baseline for all the dependency deprecations: https://symfony.com/doc/current/components/phpunit_bridge.html#baseline-...
But the best scenario is to just create an issue about this in the dependency that trigger the deprecation. Especially with Guzzle I'm quite confident that a quick decision and fix can be made (edited)

nicolasgrekas 33 minutes ago
Making patches to vendors is definitely the way to go. I've already done so for deps of Symfony (aka Doctrine repos libs) (edited)

The docs for the new feature are now online - https://symfony.com/doc/5.4/setup/upgrade_major.html#upgrading-to-symfon...

Should we just add deprecation skips for upstream packages that are not our responsibility? While Symfony has good intentions here, there is no guarantee that the return types declared in docblocks are 100% correct, especially in third-party code that isn't even planning on adding return types any time soon.

For what it is worth: We have been publishing more information about typing in Symfony 6 and how we envision preparing the PHP OSS community for more typing and Symfony 6 compatibility: https://symfony.com/blog/preparing-your-apps-and-bundles-for-symfony-6

We have also been submitting PRs to various OSS libraries that caused issues (more specifically, I checked the projects I'm working on and submitted PRs to the packages causing type deprecations). These were all merged pretty quickly, among them is the one mentioned as the cause for this issue: https://github.com/guzzle/psr7/pull/444

The patch for suppression deprecation warnings from Symfony 5.4 for deprecations that are from non Symfony packages.
If we want to do a release D9.4 with Symfony then this is needed in D9.4 too.

The code is from #3161889-188: [META] Symfony 6 compatibility from @longwave.

Adding issue credits

Committed 36f09c0 and pushed to 9.4.x. Thanks!