Skip to content

Add opt-in property fallback for ArrayAccess values in the default field resolver - #1960

Draft
spawnia wants to merge 5 commits into
masterfrom
reintroduce-array-access-property-fallback
Draft

spawnia wants to merge 5 commits into
masterfrom
reintroduce-array-access-property-fallback

Conversation

@spawnia

@spawnia spawnia commented Jul 29, 2026 •

Copy link
Copy Markdown
Collaborator

Proof of concept, kept as a draft for comparison with nuwave/lighthouse#2687.

The default field resolver reads properties of an \ArrayAccess object only when its class implements the new marker interface GraphQL\Executor\ArrayAccessPropertyFallback.
Plain \ArrayAccess values keep the behavior of v15.37.2, and the regression test from #1958 stays.

A marker interface is the only listed option that avoids every breakage from the survey

The options come from #1960 (comment).

  • Marker interface: the owner of a class decides.
    Laravel collections, Eloquent models and Drupal FieldItemList do not implement it, so nothing changes for them.
    It is additive, so it can ship in a minor version.
  • Flag on Executor: it is global per schema.
    Once enabled, it still calls Collection::__get, which throws, and still reaches Drupal entities through FieldItemList.
  • Reflection in a major version: it reads only declared public properties.
    That still exposes exists, incrementing and wasRecentlyCreated on Eloquent models.

The only public API change is the new interface.
It extends \ArrayAccess, so implementing it replaces implements \ArrayAccess.

Tests cover each case from the survey

tests/Executor/ArrayAccessPropertyFallbackTest.php checks that plain \ArrayAccess values:

  • hide public properties when an attribute is null, like Eloquent exists
  • never call a __get that throws, like Laravel Collection
  • ignore __isset/__get that forward to another object, like Drupal FieldItemList
  • ignore a __typename property in ReferenceExecutor::defaultTypeResolver

For values that opt in, offsets win over properties, and properties fill missing or null offsets, including __typename.
The four plain cases fail against the unconditional fallback that shipped in v15.37.0.

🤖 Generated with Claude Code

spawnia added 2 commits July 29, 2026 10:37
Restores #1531, reverted in
v15.37.1 because it exposed internal object state as field values.

Also removes the regression test added by the revert, which pinned the
behavior this change undoes.
@spawnia spawnia added the breaking change Warrants a major version bump, deferred to the next major release label Jul 29, 2026
@spawnia
spawnia requested a review from Copilot July 29, 2026 08:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reintroduces (after a prior revert) default resolver behavior that, for values implementing \ArrayAccess, falls back to PHP property access when the requested offset is not available—affecting Executor::defaultFieldResolver() and also __typename lookup in the default type resolver.

Changes:

  • Update Utils::extractKey() to prefer array access but fall back to property access for \ArrayAccess objects.
  • Adjust ExecutorTest coverage to assert property fallback behavior for \ArrayAccess values and remove the prior regression test that pinned “no property access”.
  • Add an Unreleased changelog entry describing the behavior change.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Utils/Utils.php Changes extractKey() resolution order to add property fallback for \ArrayAccess.
tests/Executor/ExecutorTest.php Updates default resolver test expectations to include property fallback for \ArrayAccess.
CHANGELOG.md Documents the behavior change in the Unreleased section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Utils/Utils.php Outdated
Comment thread CHANGELOG.md
…ccess-property-fallback

# Conflicts:
#	CHANGELOG.md
@spawnia

spawnia commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator Author

I surveyed public consumers to settle the open question of whether the unconditional property fallback is acceptable.
The data says no: it breaks Laravel collections, leaks Eloquent state, and has little demand.

What breaks

Laravel Collection throws.
Collection defines __get but not __isset, so $value->{$key} ?? null calls __get directly.
EnumeratesValues::__get throws for any key that is not a higher order proxy name.
Any missing or null key on a collection parent value now fails the field.
Keys such as sum or map return a HigherOrderCollectionProxy.
Lighthouse CI did not cover this.

Eloquent models leak framework state.
The fallback fires when an attribute is null.
It then returns public properties: the ones apps declare, but also exists, incrementing, timestamps, wasRecentlyCreated and preventsLazyLoading.
That is the second failing Lighthouse test, testPrefersAttributeAccessorNullThatShadowsPhpProperty, next to testPhpProperty: https://github.com/nuwave/lighthouse/actions/runs/30370354427
Accessors that return null run twice, on top of #759.
Both Lighthouse (13M downloads, ResolverProvider) and rebing/graphql-laravel (8.5M downloads, Type) route models through Executor::defaultFieldResolver.

Drupal graphql 5.x can expose entities.
It falls back to Executor::defaultFieldResolver (ResolverRegistry).
Core FieldItemList implements \ArrayAccess and forwards __isset/__get to its first item.
When a producer returns a raw list, entity resolves to the referenced entity without an access check.

__typename changes too.
ReferenceExecutor::defaultTypeResolver reads __typename through Utils::extractKey(), also when resolveType returns null.
Overblog without resolveType and Drupal reach that path.

What is unaffected

API Platform, Overblog fields, GraphQLite, Silverstripe, ecodev/graphql-doctrine, Aimeos and Craft (on a 14.x fork) install their own default resolvers.
WPGraphQL models use magic __get but do not implement \ArrayAccess.
Magento DataObject and Pimcore ArrayObject descriptors reach extractKey(), but the fallback yields the same value or null.

Demand

I found 2 requests in 6 years, and both requesters already have working custom resolvers:

nuwave/lighthouse#2687 asks for native PHP properties on Eloquent models, which Lighthouse can handle in its own resolver.
Code search found no public resolver that rebuilds "offset, then property" for \ArrayAccess.
Nobody reported breakage after v15.37.0, but v15.37.1 followed a day later, so that silence says little.

Options

The first option fits the data best.

Only values implementing ArrayAccessPropertyFallback fall back to
properties. Plain \ArrayAccess values such as Laravel collections,
Eloquent models and Drupal field lists keep reading offsets only.

Restores the regression test from #1958.

🤖 Generated with Claude Code
@spawnia spawnia changed the title Reapply reading properties of objects that implement \ArrayAccess Add opt-in property fallback for ArrayAccess values in the default field resolver Sep 28, 2026
@spawnia spawnia removed the breaking change Warrants a major version bump, deferred to the next major release label Sep 28, 2026

This branch has not been deployed

No deployments
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.

2 participants