fix: allow private and static trait methods on $this in test closures - #19
MrPunyapal merged 2 commits into
Conversation
Pest flattens a trait bound through uses() onto the generated test case, so a test closure can call the trait's private methods and its protected or private static methods. ProtectedMethodCallIgnoreExtension only covered method.protected on a $this->foo() call, so method.private, staticMethod.protected and staticMethod.private were still reported. The extension now also covers $this::foo() static calls. Private methods are allowed only when they are declared by a trait, so a private method inherited from a parent class or called on another object is still reported. Fixes pestphp/pest#1945 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
MrPunyapal
left a comment
There was a problem hiding this comment.
Thanks for the write-up, this is close. One thing blocks it.
isTrait() is broader than the reason given for it
The reasoning is right: a private member is only flattened onto the generated class when a trait is bound through uses(), and a private method inherited from a parent class is not reachable at run time. The runTest() case is covered because PHPUnit\Framework\TestCase is not a trait.
getDeclaringClass()->isTrait() does not make that distinction. It is also true when the trait is used by the bound test case class, where the private method is declared on that parent rather than on the generated class:
class TraitUsingTestCase extends TestCase
{
use VisibilityTrait;
}uses(TraitUsingTestCase::class);
it('...', function (): void {
$this->privateTraitMethod();
});Pest binds the closure to the generated class (Testable.php:508, $this::class) and PHP private access is scope based, so the child scope cannot reach the parent's private member. I ran this against the branch:
Call to private method Tests\Type\Fixtures\TraitUsingTestCase::privateTraitMethod() from scope P\C\...\ProbeTest
CallMethodsRule reports nothing for the same file.
Comparing against the binding list fixes it
The flattened traits are already resolved, PestTestCaseType::resolve() puts them in PestTestCaseWithTraitsType::$traitNames. Checking the declaring class against that list keeps every case this PR is about and rejects the one above:
uses(CustomTestCase::class, VisibilityTrait::class)gives aPestTestCaseWithTraitsTypewithVisibilityTraitin the list, ignoreduses(TraitUsingTestCase::class)gives a plainObjectTypewith no traits bound, reported$this->runTest()hasTestCaseas the declaring class, reported
pest()->extend(...)->use(Trait::class)->in(...) lands in the same list, so that keeps working too.
A test next to the existing runTest() one for this case would close it.
Two smaller notes
ProtectedMethodCallIgnoreExtension now handles method.private, staticMethod.protected and staticMethod.private. Worth renaming the class and the file before it lands.
Private trait constants are the same flattening story and are still reported. With uses(CustomTestCase::class, StateTrait::class) and a private const NAME in the trait, $this::NAME reports classConstant.private and works at run time. Not a reason to hold this, but pest#1945 is not closed until it is.
Checks
518 passed, phpstan 0 errors, rector 0 changed files. Pint fails on my machine on 5.x too, same files, CRLF checkout, so I did not count it.
Address review on pestphp#19: - Compare a private member's declaring class against the traits uses() binds (PestTestCaseWithTraitsType::getTraitNames()) instead of asking isTrait(), so only the traits Pest flattens onto the generated class are allowed. - Cover classConstant.protected and classConstant.private on $this::NAME, which the same flattening makes reachable at run time. - Rename ProtectedMethodCallIgnoreExtension to TestCaseMemberVisibilityIgnoreExtension, since it now covers private methods, static calls and constants. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for the careful review. All three points are addressed in 98335bf, and the PR description is updated to match. Trait-list check. The private forms now compare the declaring class against Rename. The class is now Private trait constants. On the uses(TraitUsingTestCase::class);
it('probe', function (): void {
$this->privateTraitMethod(); // not reported, on 5.x and on this branch
$this::privateStaticTraitMethod(); // not reported, on 5.x and on this branch
$this->doesNotExist(); // method.notFound, so the closure is analysed
$this->runTest(); // method.private, so visibility is checked in this closure
});
The trait-list check is still the right condition, and it is what the branch uses now. But closing that case needs the closure to be analysed as the generated subclass rather than the bound class. That would equally affect a private method declared directly on the bound test case, which is likewise not reported today. It seemed outside this PR. Happy to open a separate issue for it on pestphp/pest if that is useful. Checks on this branch (macOS): |
|
thanks |
|
My bad, the blocking point in my review was wrong. My fixture was malformed, PHPStan could not reflect the test case class, so the binding was dropped and |
Fixes pestphp/pest#1945.
The problem
Pest flattens a trait bound through
uses()onto the generated test case, so a test closure can reach every member of that trait. #9 and #10 madePestTestCaseWithTraitsTyperesolve those members, which fixed themethod.notFoundhalf (pestphp/pest#1815). These forms still fail analysis but work at run time:PestTestCaseWithTraitsTypereturns the trait's own reflection, so each member is checked against the trait's declared visibility. The old ignore extension accepted onlymethod.protectedon aMethodCall.The change
This extends the ignore extension rather than the composite type, the first of the two shapes in pestphp/pest#1945. It is the mechanism the plugin already uses for protected test-case methods. Reporting different visibility from
PestTestCaseWithTraitsTypewould mean wrappingExtendedMethodReflectionandClassConstantReflection, a much larger change.ProtectedMethodCallIgnoreExtensionis renamedTestCaseMemberVisibilityIgnoreExtension. It now handles:method.protectedandmethod.privateon$this->foo();staticMethod.protectedandstaticMethod.privateon$this::foo();classConstant.protectedandclassConstant.privateon$this::NAME.The existing guards are unchanged: inside an anonymous function,
$thisdefined, and$thisaPHPUnit\Framework\TestCase.uses()binds, read from the newPestTestCaseWithTraitsType::getTraitNames(). Only those traits are flattened into the generated class. A private member inherited from a parent class, such asPHPUnit\Framework\TestCase::runTest(), or one reached on any object other than$this, is still reported.Tests
tests/Rules/TraitMemberVisibilityTest.phpuses the existingcustom-testcase-extension.neonsetup and has 6 tests:trait-visibility-calls.phpbinds a test case and a trait withuses()and uses every form above fromit()andbeforeEach()closures.CallMethodsRule,CallStaticMethodsRuleandClassConstantRuleeach expect no errors.trait-visibility-errors.phpcovers what must still be reported:$this->runTest(), and private trait methods and a private trait constant reached through a different object (VisibilityTraitUser).Each change below was made to the extension by hand and the test suite rerun:
runTest()test failsrunTest()test fails$thisreceiver check removedThe last row survives because every protected member that can raise an error here belongs to a bound trait, which the trait-list check accepts anyway. The branch keeps the extension's previous
method.protectedbehavior, so nothing reported today starts being reported.Checks
All green on this branch, on macOS:
pest: 520 passed, 636 assertionsphpstan analyse: 0 errorsrector --dry-run: 0 changed filespint --test: passedIn a real suite
This was found in a suite whose Feature tests bind a support trait with 56 private and 23 protected methods, 18 of them static. That suite currently works around the problem by adding the trait to a static-analysis stub of the base
TestCase. Removing that stub entry and using this branch's extension (on v5.2.0, the release that suite installs) takes the suite from 190method.private,staticMethod.protectedandstaticMethod.privateerrors atlevel: maxto 0. The only remaining error istrait.unused, which is expected because the trait is no longer used inside a class.Generated with Claude Code.