Provide extension point to allow customizing the classification of deprecations

[stof]

This is a follow-up of my comment in #6489 (comment) as we agreed with @sebastianbergmann to keep it out of scope of that previous PR.

There are some cases where a deprecation being triggered by a class does not actually mean this deprecation is defined by that class, or that the reason to trigger it is related to the PHP caller of that class. Such use cases are currently classified badly (often being classified as indirect while they should be direct, the opposite case is very rare). Such bad classification cause issues for projects that want to fail on deprecations that they can act on but not on indirect deprecations (made worse by #6484 as they cannot display deprecations that they don't fail on).

I identified 2 cases that would benefit from such extension points:

1. the DebugClassLoader of symfony/error-handler triggers some deprecations related to class definitions themselves. This is done based on declarative phpdoc tags in the parent class, implemented interfaces and used traits. Such deprecations are semantically meant to be triggered from the file defining the class definition instead of from the class loader. For some of them, we consider them as being triggered from the file defining the parent class with the class definition as caller. This use case was reported in Dealing with deprecations from Symfony DebugClassLoader #6435
2. config file loaders that are processing config files might trigger deprecations due to the content of the config being processed. As far as the classification is concerned, we should treat the config file as the caller of the loader (as if the config file that was imported was actually doing something like $configLoader->process($myConfig)), which would allow classifying the deprecation as direct if the config file is part of the project source code (the case of config files in other formats than PHP would probably also related to includeInCodeCoverage attribute for <directory> and <file> children of <source> #6517, so that such non-PHP file can be considered project files rather than vendor files)

The deprecation reporting feature of symfony/phpunit-bridge (targeting PHPUnit 9 and older) was implementing the logic for the first case. We never implemented logic for the second case (but we were still displaying indirect deprecations by default even when not failing on them, so bad classification for them was less impactful).

Hardcoding specific support for the DebugClassLoader of symfony/error-handler in PHPUnit itself is probably not an option (as being too specific). However, Symfony already has a PHPUnit extension (targeting PHPUnit 11+) in the symfony/phpunit-bridge so we could ship this logic in Symfony if we have extension points allowing to do that.

I see 2 possible options for such extension points:

• allowing to customize the logic determining the caller and/or the callee
• allowing to customize the categorization of the caller and callee directly (i.e. the Code enum case, as introduced in Classification of self/direct/indirect deprecation triggers is not aligned with Symfony's bridge for PHPUnit #6489).

In order to ensure that the classification is consistent, I would suggest going for the first option, so that PHPUnit remains in control of how files are classified (ensuring the PHPUnit config is respected for that classification).

Here are the requirements I see for this API:

• making it possible for multiple extensions to register for this extension point
• making it possible for an extension to abstain from processing a given deprecation (letting next extensions a chance to process it)
• (maybe) stopping the processing of next extensions once an extension has applied an override already (to make it simpler to reason about how this behaves)
• the extension must be able to replace the caller, the callee or both (could be implemented in a way always requiring to return both, if the input contains the caller and callee determined by the PHPunit ErrorHandler to use them when wanting to replace only 1)
• the extension must have access to the stack trace of the deprecation to be able to use it in its processing
• the stack trace should include arguments (i.e. not use the DEBUG_BACKTRACE_IGNORE_ARGS flag when getting it, as done today), as the logic to determine the new caller and callee will generally depend on them.

The use cases I described are all related to the handling of E_USER_DEPRECATED, not to E_DEPRECATED which is triggered by the PHP runtime.

[mpdude]

Maybe one possible extra use case is a Symfony functional test, where the (own) test code calls an (own) Controller action, but this call works through the Symfony HttpKernel, which is the direct caller of the action method.

[sebastianbergmann]

CC @derrabus

[stof]

@mpdude anything triggering a deprecation where the callee is own code is classified as self deprecation since #6489. So I don't think this is a case needing such extension point.

[sebastianbergmann]

Thank you, @stof, for the thorough feature request.

In order to ensure that the classification is consistent, I would suggest going for the first option, so that PHPUnit remains in control of how files are classified (ensuring the PHPUnit config is respected for that classification).

I agree.

Here is my current idea:

Introduce an interface, let us call it IssueTriggerResolver for now:

interface IssueTriggerResolver
{
    /**
     * Return null to defer to the next categorizer in the chain.
     *
     * @param list<array{file?: string, line?: int, class?: class-string, function?: string, type?: string}> $trace
     *
     * @return null|array{callee: ?non-empty-string, caller: ?non-empty-string}
     */
    public function resolve(array $trace, string $message): ?array;
}

Move the current logic to a new DefaultIssueTriggerResolver class that implements IssueTriggerResolver.

Add an array property of IssueTriggerResolver objects that defaults to [new DefaultIssueTriggerResolver] to ErrorHandler as a chain of responsibility that works like you describe.

Introduce a <issueTriggerResolvers> child of <source> with <issueTriggerResolver className="..."/> children to configure an arbitrary number of custom IssueTriggerResolver objects. They are prepended to the array property of IssueTriggerResolver objects in the ErrorHandler in the order they appear in phpunit.xml. This means that if none of the custom IssueTriggerResolver objects make a decision then the DefaultIssueTriggerResolver will decide.

Does that make sense?

[stof]

Should the resolver have access to $errorFile that the ErrorHandler has ?

Regarding the suggested type for the $trace argument, I have 2 feedbacks:

• it describes file as string while the return value expects non-empty-string. This will force implementations to do additional logic to satisfy static analyzers. I suggest being consistent regarding string vs non-empty-string for file paths
• it does not seem to provide arguments in the trace

Also, should the return value be an array shape or a value object ?

[stof]

Another question: should an extension be able to register such resolver or do we require the end-user to register them separately in the config file ?

[sebastianbergmann]

Should the resolver have access to $errorFile that the ErrorHandler has?

Do you think it is needed? I am not against adding this per se, but if we should have a use case for that.

The DefaultIssueTriggerResolver does not need $errorFile:

public function resolve(array $trace, string $message): array
{
    return [
        'callee' => $trace[0]['file'] ?? null,
        'caller' => $trace[1]['file'] ?? null,
    ];
}

The above is from a local branch with a proof-of-concept implementation.

[sebastianbergmann]

Also, should the return value be an array shape or a value object?

An array shape, as can be seen in the code in #6530 (comment)

[sebastianbergmann]

should an extension be able to register such resolver or do we require the end-user to register them separately in the config file

I do not think that an extension should be able to register an IssueTriggerResolver, nor should an extension be required to do so.

This is what I currently have in my local branch:

<source>
    <include>
        <directory>src</directory>
    </include>

    <issueTriggerResolvers>
        <issueTriggerResolver className="..."/>
    </issueTriggerResolvers>
</source>

[sebastianbergmann]

it does not seem to provide arguments in the trace

Good catch, I forgot about that. Probably because it is not needed for the DefaultIssueTriggerResolver.

[sebastianbergmann]

it describes file as string while the return value expects non-empty-string. This will force implementations to do additional logic to satisfy static analyzers. I suggest being consistent regarding string vs non-empty-string for file paths

👍 for consistency. However, I'm afraid the consistency will have to be string instead of non-empty-string. Or am I missing something? Will look into this.