Skip to content

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Oct 8, 2025

Follows up on #1776 (comment)

@spawnia spawnia requested a review from ruudk October 8, 2025 06:51
@spawnia
Copy link
Collaborator Author

spawnia commented Oct 8, 2025

Tests pass, I don't see how this change could be problematic.

@ruudk
Copy link
Collaborator

ruudk commented Oct 8, 2025

Since the ReferenceExecutor is not final, one could be extending it.

For example:

final class TestReferenceExecutor extends ReferenceExecutor {
    protected function completeAbstractValue(
        AbstractType $returnType,
        \ArrayObject $fieldNodes,
        ResolveInfo $info,
        array $path,
        array $unaliasedPath,
        &$result,
        $contextValue)
    {
        return parent::completeAbstractValue(
            $returnType,
            $fieldNodes,
            $info,
            $path,
            $unaliasedPath,
            $result,
            $contextValue,
        );
    }
}

This would fail now:

Declaration should be compatible with ReferenceExecutor->completeAbstractValue(returnType: \GraphQL\Type\Definition\AbstractType|\GraphQL\Type\Definition\Type&\GraphQL\Type\Definition\AbstractType, fieldNodes: \ArrayObject, info: \GraphQL\Type\Definition\ResolveInfo, path: int[]|string[], unaliasedPath: int[]|string[], result: mixed, contextValue: mixed) 

@spawnia
Copy link
Collaborator Author

spawnia commented Oct 8, 2025

We do mark public elements of our API with @api, so I would categorize this as internal and non-breaking.

@ruudk
Copy link
Collaborator

ruudk commented Oct 8, 2025

In that case, let's ship this! Way better indeed.

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