Fix phpstan/phpstan#7088: Accessing SimpleXMLElement nodes using braces gives mixed type#5406
Fix phpstan/phpstan#7088: Accessing SimpleXMLElement nodes using braces gives mixed type#5406phpstan-bot wants to merge 2 commits intophpstan:2.1.xfrom
Conversation
…turns mixed
- When accessing SimpleXMLElement properties via $prop->{$x} with a non-constant
string name, the type was incorrectly resolved as mixed instead of SimpleXMLElement|null
- Added fallback in PropertyFetchHandler::resolveType() to query the var type's
property reflection for non-constant string property names
- New regression test in tests/PHPStan/Analyser/nsrt/bug-7088.php
VincentLanglet
force-pushed
the
create-pull-request/patch-2l111d3
branch
from
9e33578 to
721d265
Compare
|
While using a hardcoded constant I think it's good enough but we might prefer ask validation on ondrej. WDYT ? |
|
|
||
| if ($nameType->isString()->yes()) { | ||
| $fetchedOnType = $scope->getType($expr->var); | ||
| $returnType = $this->propertyFetchType($scope, $fetchedOnType, '__phpstan_dynamic_property', $expr); |
There was a problem hiding this comment.
what is this magic __phpstan_dynamic_property constant about? I don't get why we need it.
if we need it, it might make sense to extract it into a private constant and give it a name, which makes its use-case obvious.
There was a problem hiding this comment.
Done. Extracted the magic string '__phpstan_dynamic_property' into a private constant DYNAMIC_PROPERTY_NAME with a docblock explaining its purpose: it's a representative property name used when resolving dynamic property access ($obj->{$expr}), so that PropertiesClassReflectionExtension implementations (like SimpleXMLElement) that accept any property name can return the correct type. Both make phpstan and the bug-7088 test pass.
Address review feedback: the magic string '__phpstan_dynamic_property'
is now a private constant with a docblock explaining its purpose.
It serves as a representative property name for dynamic property access
resolution ($obj->{$expr}), allowing PropertiesClassReflectionExtensions
that accept any property name to return the correct type.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
Accessing
SimpleXMLElementproperties using dynamic names ($prop->{$x}) was incorrectly resolved asmixedinstead ofSimpleXMLElement|null. Static property names ($prop->foo) and constant string expressions ($prop->{'foo-bar'}) already worked correctly.Changes
src/Analyser/ExprHandler/PropertyFetchHandler.phpfor non-constant string property names: when the name is a string type but not a constant string, query the var type's property reflection to determine the typetests/PHPStan/Analyser/nsrt/bug-7088.phpRoot cause
In
PropertyFetchHandler::resolveType(), when the property name expression is not anIdentifierand not a constant string, the code immediately returnedMixedType. For classes likeSimpleXMLElementthat have aPropertiesClassReflectionExtensionaccepting any property name, we can still determine the correct type by querying the property reflection with a representative property name.Test
Added
tests/PHPStan/Analyser/nsrt/bug-7088.phpthat verifies all three property access patterns onSimpleXMLElementresolve to(SimpleXMLElement|null):$prop->foo(static name)$prop->{'foo-bar'}(constant string expression)$prop->{$x}(dynamic variable name)Fixes phpstan/phpstan#7088