NEW: Refactor OperationScaffolder subclasses to use resolved type name #182
Conversation
|
The problem with this is that getTypeName() / getResolvedTypeName() both ignore PREFER_UNION which requires the manager to resolve. getType()->name could still be different to either of the return values of those methods. On the other hand, it's still not worse than the status quo, it just doesn't address the inconsistencies. I also thought about binding manager to the types directly, so we have a getManager() on the scaffolders instead of having to pass it in. |
|
Also, we should be careful about "We have 5 methods to get the type name so fix it by adding a 6th". :P |
There was a problem hiding this comment.
I tested this locally by implementing a simple graphql service in framework test. I've tested:
- letting the typename be inferred from the DataObject name,
- specifying different typenames via a
typeNamesmap on the Manager YML config, - explicitly specifying a typename via a
scaffolding_providersimplementation.
I think @tractorcow concern is partially mitigated by making the new methods protected. ResolveTypeName and getDataObjectTypeName are mostly implementation details that you don't need to be aware off when interacting with the scaffolder classes.
| * | ||
| * @return string | ||
| */ | ||
| protected function typeName() | ||
| public function getDataObjectTypeName() |
There was a problem hiding this comment.
Does this need to be public? From the usage above, it looks like this is mostly dealing with internal logic consideration.
If it turns out we need it to be public later, it's easy to make it public.
There was a problem hiding this comment.
Maybe add a @throws statement.
There was a problem hiding this comment.
True, but would make this an API breakage, and since it's essentially a bug fix, I really would love to get it into 3.
There was a problem hiding this comment.
Why is this an API breakage? There's no getDataObjectTypeName right now.
There was a problem hiding this comment.
And introducing APIs to fix bugs is fine in Semver.
There was a problem hiding this comment.
Removing the protected method is risky but also fine in terms of semver. They're not part of our "public API"
There was a problem hiding this comment.
This is targeted at version 3. Which hasn't been release yet. Or do we consider RC to be part of our semver commitment?
|
FYI I was using this PR on FrameworkTest. |
Resolves: #176