test: cover non-final class branch of the higher-order property crash - #5
Open
thimarsola wants to merge 1 commit into
Open
Conversation
`ObjectType::hasProperty()` returns `maybe` on two independent branches:
`allowsDynamicProperties()` (a `__get()` method) and `!isFinal()`. The
regression tests added with the crash fix only covered the first one.
The second branch is reachable whenever the object type does not come from
a `new` expression — PHPStan marks those class reflections as final via
`ClassReflection::asFinal()`, which is why `new NonFinalObject` answers `no`
while a parameter typed `NonFinalObject` answers `maybe`. Reverting the
try/catch in `resolvePropertyType()` makes these tests fail with:
Property $name was not found in reflection of class Tests\Type\Fixtures\NonFinalObject.
The fixture is abstract so it stays non-final under Pint's `final_class`
rule, which also matches the real-world trigger (`Eloquent\Model` is an
abstract non-final class).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TvRLUyRV47CJr9NfpxWg2M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #4. Tests only — no production code changed.
Gap
ObjectType::hasProperty()returnsmaybeon two independent branches:The regression tests shipped with #4 only cover the first one (
__get()onMagicPropertyObject) plus the union case. The!isFinal()branch — a plainnon-final class without the property — was left uncovered, even though it
reaches the exact same
MissingPropertyFromReflectionExceptioninresolvePropertyType().Why
new NonFinalObjectwould not have caught itWorth noting for anyone writing a similar test: a
newexpression is notenough.
NewHandler::…builds the resulting object type from$classReflection->asFinal(), sohasProperty()answersnoand the codenever reaches the reflection lookup:
hasProperty('name')expect(new NonFinalObject)->nameno— bails out earlyexpect($object)->name(param typedNonFinalObject)maybe— reachesgetProperty()The new tests therefore take the object as a function parameter, which is also
how it shows up in practice (type hints, typed properties,
@var).Verification
Reverting the
try/catchfrom #4 makes the new tests fail with the originalinternal error, isolated from the
__get()case:Tests added
testNonFinalClassMissingPropertyDoesNotCrash— resolves tomixedtestNonFinalClassMissingPropertyChainDoesNotCrash— chain keeps itsHigherOrderExpectation<…>shape through the matchertestNonFinalClassRealPropertyStillResolves— non-regression of theyespath on a non-final class (real property still resolves to
string, notmixed)composer testis green: rector, pint, phpstan (0 errors), 468 tests /581 assertions.
Note on the fixture
Tests\Type\Fixtures\NonFinalObjectis declaredabstractrather than as aplain class. Pint's
final_classrule is enabled repo-wide with no exclusionsand would otherwise rewrite the fixture to
final, silently defeating the test.abstractalso matches the real-world trigger —Illuminate\Database\Eloquent\Modelis an abstract, non-final class.