Fix QueryScaffolder not using inheritance types

[unclecheese]

Other pull requests

For the 3 branch fix:

• NEW: Refactor OperationScaffolder subclasses to use resolved type name #182
• Compliance with new graphql resolvedTypeName() method silverstripe-versioned#176

[tractorcow]

If getTypeName() is returning the wrong value, I wonder if we can fix it there?

This logic defers auto-type scaffolding from StaticSchema, which may in some cases be the correct behaviour. It fixes one use case but breaks others.

It seems like the fundamental problem is "typeName" can't be determined without a manager. In that case, this issue is a symptom of a fundamental problem of manager-gnostic type resolution.

Looking back at 7c3980a, it seems like we introduced prefer_union = true.

Perhaps that logic needs to be available to getTypeName() as well, so it returns the union type name?

Do both getType() / getTypeName() need manager to be present?

[maxime-rainville]

What Damian said.

[unclecheese]

The follow up discussion on this happened on Slack:

tl;dr rewrite all these methods to be more semantically correct.

unclecheese [12:11 PM]
@tractorcow how do you feel about clearing up the semantics a bit?

getTypeName() // gets the type name as assigned to the scaffolder. Falls back on typename for $dataObjectClass

getResolvedTypeName(Manager $manager) // gets the typename that will actually be used in the schema based on manager state

exact naming TBD, but you get the gist

tractorcow [12:35 PM]
Yeah that would be ideal IMO

we can't remove getTypeName() but we should be using the resolved name where possible right?
we'd have like 4-5 different "get name" failovers lol

by this point

I dunno, I guess I'm really just pointing out problems without giving solutions, I'm sorry
Normally I'd properly investigate and come up with something

unclecheese [12:44 PM]
I'd also love to get rid of DataObjectTrait.. that introduces yet another ambiguous method name `typeName()`

tractorcow [12:49 PM]
hah yeah

that was a holdover from the proof of concept

[unclecheese]

The ideal fix for this involves refactoring that is much more appropriate for the 3 branch. Recommend we merge this into 2 in spite of its flaws. A more thorough fix for the 3 branch is here: #182

[tractorcow]

Issue is that typeName() from DataObjectTypeTrait bypasses the logic in StaticSchema::fetchFromManager()

Solution is to add getResolvedTypeName() that uses the manager to resolve the name always.

Suggestion:

• Add $manager arg to getTypeName(). Make it optional for now, but warn if missing (deprecation notice).
• Alternatively, add new getResolvedTypeName(), deprecate entire getTypeName() method.
• Validate that getType() and getTypeName() both behave consistently. You may want to refactor part of StaticSchema::fetchFromManager() a little so that you can use the schema to resolve either the name, or the type directly.
• Make sure that usages of getTypeName() always passes is the appropriate $manager instance. You may need to inject it into a few top level entry points to the code.

[tractorcow]

As per last comment.

[unclecheese]

A more thorough fix, with API changes is against the 3 branch in #182.

I don't see the case for passing Manager to getTypeName. Maybe as an optional argument, but we certainly shouldn't throw a deprecation warning. We need to afford the flexibility use any arbitrary name you want, and not couple it so tightly to dataobjects.

It gets really confusing, and I've tried to clean up the semantics a bit in the 3 PR, but basically:

• getTypeName() a simple getter for a property that is set via setTypeName() (or a constructor arg). It's arbitrary, and manager-unaware.
• getDataObjectTypeName() - Generate a typename based on the dataobject being used. This isn't necessarily the same as the type that is returned by the resolver. For all wants and purposes, it too, is arbitrary. It's for naming purposes, so we get readProducts instead of readProductsWithDescendants.
• getType(Manager) is used to get the concrete type that is returned by the resolver. In theory you should never have to know the name of this type. It's computed just-in-time when the scaffolder is added to the manager, and the manager state is guaranteed to be frozen.

[maxime-rainville]

@ScopeyNZ @robbieaverill We got a bit of a deadlock on this one and we're not quite sure how to proceed. Would you care to throw your two cents on this?

[robbieaverill]

I'm not familiar enough with this part of the code TBH. I can say that it's preventing us from using dynamic fragments, which we'll need to start using at some point for the CWP modules. It's also come up in Stackoverflow, and apparently this PR fixes the problem: https://stackoverflow.com/questions/53184168/silverstripe-graphql-error-when-querying-types-with-descendants

My assumption is that the blocking comments here are mostly semantic

[maxime-rainville]

Following some back and forth. I'm OK with this being merge.

Calling parent::getTypeName() from getType() is weird, but in the context that this is a low level abstract class and that this will only affect the 2 branch we can probably live with it.

Lack of response, approved by another core commmiter

[tractorcow]

Sorry @unclecheese I'll just leave this to you. I don't have much time these days to get too deep into graphql. =(