Skip to content

If an object implements \ArrayAccess, check both array value and property#1531

Open
shish wants to merge 2 commits into
webonyx:masterfrom
shish:patch-1
Open

If an object implements \ArrayAccess, check both array value and property#1531
shish wants to merge 2 commits into
webonyx:masterfrom
shish:patch-1

Conversation

@shish

@shish shish commented Feb 25, 2024

Copy link
Copy Markdown

I have an object which has known properties accessible via properties, and some extra bits added at runtime accessible via \ArrayAccess syntax. graphql-php sees the \ArrayAccess and now it thinks that all my fields are null :(

My specific use-case is that I had a class like

#[Type]
class Post {
    #[Field]
    public string $title = "Untitled";
}

and everything was working fine, but then I turned it into

#[Type]
class Post implements ArrayAccess {
    #[Field]
    public string $title = "Untitled";

    // ...
    // implementing ArrayAccess methods for internal use
    // only and not exposing them to graphql at all
    // ...
}

and suddenly any query which referred to the "title" field started throwing Cannot return null for non-nullable field "Post.title"

Comment thread src/Utils/Utils.php Outdated
@spawnia

spawnia commented Feb 26, 2024

Copy link
Copy Markdown
Collaborator

Please extend testDefaultResolverGrabsValuesOffOfCommonPhpDataStructures.

Comment thread src/Utils/Utils.php Outdated
@shish
shish force-pushed the patch-1 branch 2 times, most recently from 67585eb to f2a1729 Compare February 26, 2024 15:54

@spawnia spawnia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I plan to include this when I start working on the next major version.

@spawnia

spawnia commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Hey, tried to update this branch against master and ran into an issue: your fork (shish/graphql-php) seems to be archived, so it's read-only now, even for you.

I already resolved the merge conflict against master locally, so once you unarchive the fork (or open a fresh PR from an active one), updating it should be quick.

@shish

shish commented Jul 20, 2026

Copy link
Copy Markdown
Author

Unarchived now, thank you for keeping it updated :)

# Conflicts:
#	tests/Executor/ExecutorTest.php

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

This PR adjusts the default field resolution behavior so that objects implementing \ArrayAccess are no longer treated as array-only containers: if the requested key is not available via ArrayAccess, graphql-php will now fall back to reading an object property of the same name. This aligns ArrayAccess-implementing objects with typical PHP usage where they can expose both real properties and dynamic “extra” values.

Changes:

  • Update Utils::extractKey() to check object properties when a \ArrayAccess lookup does not yield a value.
  • Extend the executor resolver test to cover an \ArrayAccess object with a real property (setProperty) and ensure it resolves correctly.

Reviewed changes

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

File Description
tests/Executor/ExecutorTest.php Adds coverage proving the default resolver can read a real property from an ArrayAccess object.
src/Utils/Utils.php Updates key extraction to fall back to property access for ArrayAccess objects.

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants