Test for PHP property access #2688
Conversation
| /** @see https://github.com/nuwave/lighthouse/issues/2687 */ | ||
| public function testPrefersAttributeAccessorNullThatShadowsPhpProperty(): void |
There was a problem hiding this comment.
This test is failing with the current override of the default field resolver, whereas it used to work before.
| /** @see https://github.com/nuwave/lighthouse/issues/1671 */ | ||
| public function testExpensivePropertyIsOnlyCalledOnce(): void |
There was a problem hiding this comment.
When we override the default field resolver, we should also consider this issue and fix it too.
| $property = $objectLikeValue->getAttribute($fieldName); | ||
| if ($property === null && property_exists($objectLikeValue, $fieldName)) { | ||
| $property = $objectLikeValue->{$fieldName}; | ||
| } |
There was a problem hiding this comment.
Can we find an implementation that keeps all the tests happy? Or should we bite the bullet and let testPrefersAttributeAccessorNullThatShadowsPhpProperty fail?
There was a problem hiding this comment.
Personally I think having a php property and Laravel-access attribute of the same name is an anti-pattern. I'd even argue that php properties should have priority as that is the way php itself does it. Also they are less computationally expensive to check for presence via property_exists.
The only way to check this would involve checking all the Laravel ways you can define an attribute, not sure if that is worth the effort and extra computation for something that should be evaded in the first place.
Also I don't think such a check can be 100% reliable. While I cannot think of a reason someone would do this, the field could refer to a DB column that isn't selected so it does not appear in Model::$attributes.
Something that might be valuable is adding a check in the maintenance artisan commands that check for name collisions and warn the developer. If for some reason they really need to shadow a php property with a model attribute they can create an accessor with a different name and use @rename.
2 participants
Adds a (currently failing) test for accessing php properties on Laravel models as requested in #2687