Skip to content

test: cover non-final class branch of the higher-order property crash - #5

Open
thimarsola wants to merge 1 commit into
pestphp:5.xfrom
thimarsola:test/non-final-class-higher-order-regression
Open

test: cover non-final class branch of the higher-order property crash#5
thimarsola wants to merge 1 commit into
pestphp:5.xfrom
thimarsola:test/non-final-class-higher-order-regression

Conversation

@thimarsola

@thimarsola thimarsola commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #4. Tests only — no production code changed.

Gap

ObjectType::hasProperty() returns maybe on two independent branches:

if ($classReflection->allowsDynamicProperties()) {
    return TrinaryLogic::createMaybe();
}
if (!$classReflection->isFinal()) {
    return TrinaryLogic::createMaybe();
}

The regression tests shipped with #4 only cover the first one (__get() on
MagicPropertyObject) plus the union case. The !isFinal() branch — a plain
non-final class without the property — was left uncovered, even though it
reaches the exact same MissingPropertyFromReflectionException in
resolvePropertyType().

Why new NonFinalObject would not have caught it

Worth noting for anyone writing a similar test: a new expression is not
enough. NewHandler::… builds the resulting object type from
$classReflection->asFinal(), so hasProperty() answers no and the code
never reaches the reflection lookup:

expression hasProperty('name')
expect(new NonFinalObject)->name no — bails out early
expect($object)->name (param typed NonFinalObject) maybe — reaches getProperty()

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/catch from #4 makes the new tests fail with the original
internal error, isolated from the __get() case:

Property $name was not found in reflection of class Tests\Type\Fixtures\NonFinalObject.

Tests added

  • testNonFinalClassMissingPropertyDoesNotCrash — resolves to mixed
  • testNonFinalClassMissingPropertyChainDoesNotCrash — chain keeps its
    HigherOrderExpectation<…> shape through the matcher
  • testNonFinalClassRealPropertyStillResolves — non-regression of the yes
    path on a non-final class (real property still resolves to string, not
    mixed)

composer test is green: rector, pint, phpstan (0 errors), 468 tests /
581 assertions.

Note on the fixture

Tests\Type\Fixtures\NonFinalObject is declared abstract rather than as a
plain class. Pint's final_class rule is enabled repo-wide with no exclusions
and would otherwise rewrite the fixture to final, silently defeating the test.
abstract also matches the real-world trigger — Illuminate\Database\Eloquent\Model
is an abstract, non-final class.

`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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant