Skip to content

Stop calling typeLoader for built-in scalar names#1884

Open
spawnia wants to merge 5 commits intomasterfrom
stop-type-loader-for-built-in-scalars
Open

Stop calling typeLoader for built-in scalar names#1884
spawnia wants to merge 5 commits intomasterfrom
stop-type-loader-for-built-in-scalars

Conversation

@spawnia
Copy link
Collaborator

@spawnia spawnia commented Mar 22, 2026

Summary

Fixes #1874

Calling the typeLoader for built-in scalar names (String, Int, Float, Boolean, ID) breaks downstream consumers whose typeLoaders were not designed to handle these names.
This removes typeLoader-based scalar override support, keeping only types-based overrides.

  • Remove the typeLoader scalar probe loop in getTypeMap() that iterated built-in scalar names
  • Guard loadType() against built-in scalar names so the typeLoader is never called for them
  • Convert all typeLoader-based scalar override tests to use types config
  • Add regression test verifying typeLoader is not called for built-in scalar names

Test plan

  • vendor/bin/phpunit tests/Type/ScalarOverridesTest.php — all 11 tests pass
  • vendor/bin/phpunit — full suite green (1 pre-existing unrelated failure in EnumTypeTest)
  • vendor/bin/phpstan — no errors

🤖 Generated with Claude Code

spawnia added 3 commits March 22, 2026 12:01
Calling the typeLoader for built-in scalar names (String, Int, Float,
Boolean, ID) breaks downstream consumers whose typeLoaders were not
designed to handle these names.

Remove typeLoader-based scalar override support, keeping only
types-based overrides. Guard loadType() so the typeLoader is never
invoked for built-in scalar names.

Fixes #1874

🤖 Generated with Claude Code
@ruudk
Copy link
Collaborator

ruudk commented Mar 22, 2026

How is this now going to be supported when using a type loader? With this Pr you can only do it when using a types map?

@ruudk
Copy link
Collaborator

ruudk commented Mar 22, 2026

I'd rather have an option that enables scalars via type loader. Make the default behavior in a new version. But it allows people to opt-in when using a type loader.

Also, I have the feeling that people that want custom scalar types, often also have a type loader. So making this only apply to people without a type loader does not make much sense to me.

@ruudk
Copy link
Collaborator

ruudk commented Mar 22, 2026

So basically I would vote for #1882

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: 89cd752 Previous: 317a71e Ratio
DeferredBench::benchManyNestedDeferreds 21.75 ms 12.163 ms 1.79

This comment was automatically generated by workflow using github-action-benchmark.

@spawnia
Copy link
Collaborator Author

spawnia commented Mar 22, 2026

typeLoader users can still override built-in scalars — they just use types for it instead of the typeLoader itself:

$schema = new Schema([
    'query' => $queryType,
    'typeLoader' => $myTypeLoader,
    'types' => [$uppercaseString],
]);

I added a test that proves both mechanisms work together: the typeLoader resolves custom types (like User) while types overrides the built-in String scalar, and both are applied correctly.

Also added documentation for this in the scalars docs and updated the types option description in the schema definition docs.

@ruudk
Copy link
Collaborator

ruudk commented Mar 22, 2026

Oké great let's try it out!

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

Labels

Projects

None yet

2 participants