Skip to content

collect autoloading deprecations before running testCase #6165

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

soyuka
Copy link
Contributor

@soyuka soyuka commented Mar 28, 2025

When one triggers a deprecation in a class directly like this:

<?php
trigger_error('Depreciated interface', E_USER_DEPRECATED);

interface Foo {}

This technique is notably used in many Symfony components, for example:

https://github.com./symfony/symfony/blob/82cf90aefb7e9117ebd981fec83d7546e310e52b/src/Symfony/Component/PropertyInfo/Type.php#L12-L15

The deprecation will be triggered when autoloading the class, which happens in:

$testSuite = $this->buildTestSuite($configuration);

At that point, the ErrorHandler is not registered and we loose the deprecation.

This pull request gives an idea on how it may be fixed, indeed the ErrorHandler needs a TestCase therefore I'm collecting these deprecations and triggering them once we call ErrorHandler::enable. I'm open to suggestions on how to fix this differently. Obviously I'll add tests if/once a solution is found that can be merged.

Thanks!

@@ -178,8 +178,16 @@ public function run(array $argv): int

EventFacade::instance()->seal();

$depreciations = [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this variable should be $deprecations.

@@ -422,4 +424,16 @@ private function stackTrace(): string

return $buffer;
}

public function collectGlobalDepreciations(array $deprecations): void

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this method should be collectGlobalDeprecations.

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Mar 28, 2025

I think this idea is worthwhile to pursue. Please fix the existing tests and add new tests to cover the changed/added functionality.

I do not consider this to be a bug fix, so this pull request should target the main branch instead of 11.5.

@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner labels Mar 28, 2025
@soyuka soyuka changed the base branch from 11.5 to main March 28, 2025 15:13
@soyuka soyuka force-pushed the global-deprec branch 4 times, most recently from d02f1ab to 10515dd Compare March 28, 2025 23:35
@soyuka
Copy link
Contributor Author

soyuka commented Mar 28, 2025

#5844 link I updated the code, interestingly this conflicts with the resolution of #5844 (https://github.com./sebastianbergmann/phpunit/commit/8e8776083c71277fa90403b183530c956b110b60). To mitigate this I need to use something like:

while (true) {
$previousHandler = set_error_handler(static fn () => false);
restore_error_handler();
if ($previousHandler === null) {
break;
}
$activeErrorHandlers[] = $previousHandler;
restore_error_handler();
}

The idea would be to restore only the handler that we set to avoid restoring another handler registered by third party (bootstrap or test suite configuration).

The handler that's created in the bootstrap.php of 5844 fails once restored as it's a private method... I also think it makes the code slightly more complicated then what I would like (see failure at https://github.com./sebastianbergmann/phpunit/actions/runs/14144107940/job/39629361311?pr=6165)

@soyuka soyuka force-pushed the global-deprec branch 4 times, most recently from 3a94e60 to af0dc23 Compare March 29, 2025 19:39
@@ -0,0 +1,12 @@
<?php declare(strict_types=1);

spl_autoload_register(function ($class) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reflects what happens when loading classes usually, if we do the same as in #5844 we'll trigger the deprecation during the loadBootstrapScript.

@soyuka soyuka force-pushed the global-deprec branch 3 times, most recently from 83dcd44 to 987f371 Compare April 2, 2025 08:58
@TimWolla
Copy link

Did you test this with OPcache? I came across this PR on my phone and php/php-src#17422 sounds relevant.

@soyuka
Copy link
Contributor Author

soyuka commented Apr 18, 2025

I don't use opcache

@sebastianbergmann sebastianbergmann force-pushed the main branch 4 times, most recently from 1bb9097 to 5af6639 Compare April 20, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants