Skip to content

Stop calling typeLoader for built-in scalar names#1884

Merged
spawnia merged 5 commits intomasterfrom
stop-type-loader-for-built-in-scalars
Mar 23, 2026
Merged

Stop calling typeLoader for built-in scalar names#1884
spawnia merged 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!

@spawnia spawnia merged commit 3f4d98e into master Mar 23, 2026
22 checks passed
@spawnia spawnia deleted the stop-type-loader-for-built-in-scalars branch March 23, 2026 06:55
@ruudk
Copy link
Collaborator

ruudk commented Mar 23, 2026

Testing latest dev-master in our project now. Let's wait with a tag before I confirm that it works.

@ruudk
Copy link
Collaborator

ruudk commented Mar 23, 2026

It's still not working as expected #1886

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

Labels

Projects

None yet

2 participants