ISSUE-4798: Memory leaks in TestSuite class

[kandy]

• Do not store list of classes in property
  fix: Memory leaks in TestSuite class #4798

[IonBazan]

This has been introduced in #2861 on purpose for some edge case. Let's make sure this PR does not introduce a regression.
Perhaps we could try using a static variable instead?

cc @kubawerlos.

[villfa]

In order to reduce the memory usage I propose to store only the number of declared classes instead of the whole list of declared classes:
BEFORE

$declaredClasses = get_declared_classes();
// some code
$newClasses = array_diff(get_declared_classes(), $declaredClasses);

AFTER

$numberOfDeclaredClasses = count(get_declared_classes());
// some code
$newClasses = array_slice(get_declared_classes(), $numberOfDeclaredClasses);

[kandy]

In order to reduce the memory usage I propose to store only the number of declared classes instead of the whole list of declared classes:

Based on https://www.php.net/manual/en/function.get-declared-classes.php

No particular order is guaranteed for the get_declared_classes() return value.

It's maybe not the best idea to depend on behavior that may be changed, but let's try

[villfa]

I've missed this change. Thanks

EDIT here the full quote:

Previously get_declared_classes() always returned parent classes before child classes. This is no longer the case. No particular order is guaranteed for the get_declared_classes() return value.

With this context I think we're safe.

[kandy]

@villfa I have implemented your suggestion

[sebastianbergmann]

When I apply this patch to 9.5 then I get the following test failure:

1) /usr/local/src/phpunit/tests/end-to-end/regression/GitHub/2972.phpt
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 Warning:       Test case class not matching filename is deprecated
                in /usr/local/src/phpunit/tests/end-to-end/regression/GitHub/2972/unconventiallyNamedIssue2972Test.php
+               Class name was 'PhptTestCase', expected 'unconventiallyNamedIssue2972Test'
+Warning:       Test case class not matching filename is deprecated
+               in /usr/local/src/phpunit/tests/end-to-end/regression/GitHub/2972/unconventiallyNamedIssue2972Test.php
                Class name was 'Issue2972Test', expected 'unconventiallyNamedIssue2972Test'
 
 ..                                                                  2 / 2 (100%)
 
-Time: %s, Memory: %s
+Time: 00:00.051, Memory: 4.00 MB
 
 OK (2 tests, 2 assertions)

[kandy]

@sebastianbergmann For 9.x there is one more usage of declaredClasses property in the class that should be replaced with declaredClassesPointer. see https://github.com/sebastianbergmann/phpunit/compare/master...kandy:Issue-4798-9x?expand=1#diff-ce07bbf9be7fe14e3102ab699c8a2d6c2a995bc23f8f93871fb4f3dde098dfaaR392 (line: 392)

[sebastianbergmann]

For 9.x there is one more usage of declaredClasses property

And by "9.x" you mean "8.x", right? Because that is the target of this pull request.

[kandy]

For 9.x there is one more usage of declaredClasses property

And by "9.x" you mean "8.x", right? Because that is the target of this pull request.

no 9.5, see your previous comment #4799 (comment)

[sebastianbergmann]

no 9.5, see your previous comment #4799 (comment)

My bad, too early in the morning :)

[kandy]

@sebastianbergmann What is the status of this OR? Is any further action is needed to merge it?

[sebastianbergmann]

I merged these changes into 8.5. I merged them from there to 9.5 and made the change you suggested in #4799 (comment) in e169850. The code in question does not exist in master anymore.