Support generics typed iterables for output types

[andrew-demb]

refs: #436

Unfortunately, I'm new to the GraphQLite codebase, so it is hard for me to find a clean implementation (and to know about possible drawbacks of chosen way to handle generic iterables)

[codecov-commenter]

Codecov Report

Merging #468 (a93b394) into master (da8a377) will decrease coverage by 0.02%.
The diff coverage is 85.71%.

❗ Current head a93b394 differs from pull request most recent head ef841d2. Consider uploading reports for the commit ef841d2 to get more accurate results

@@             Coverage Diff              @@
##             master     #468      +/-   ##
============================================
- Coverage     96.36%   96.34%   -0.03%     
- Complexity     1631     1635       +4     
============================================
  Files           146      146              
  Lines          4100     4103       +3     
============================================
+ Hits           3951     3953       +2     
- Misses          149      150       +1     

Impacted Files	Coverage Δ
src/Mappers/CannotMapTypeException.php	98.21% <50.00%> (-1.79%)	⬇️
src/Mappers/Parameters/TypeHandler.php	98.07% <100.00%> (+0.01%)	⬆️
src/Mappers/Root/BaseTypeMapper.php	100.00% <100.00%> (ø)
src/Mappers/Root/IteratorTypeMapper.php	96.55% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da8a377...ef841d2. Read the comment docs.

[oojacoboo]

Hey @andrew-demb. Thanks for this. Did you see #321?

[andrew-demb]

Hey @oojacoboo .
Yes, I saw.

That PR doesn't solve the problem.
IteratorTypeMapper only handle union types (Compound), so provided fix is not enough.
You can check my attempt (which passes the test with generic phpdoc) to solve the issue with IteratorTypeMapper in the first commit.

[oojacoboo]

Thanks for clarifying @andrew-demb. I think it'd be good to also have a test for the full schema parser, maybe something in tests/Integration/EndToEndTest.php or even a new test class in tests/Integration.

[andrew-demb]

@oojacoboo I found a fixture controller with different typed methods and added to it new methods with generic types.
Adding new methods bubbles up another problem with merging phpdoc+return type types.
So I added a patch to this merging process to be more accurate (it may look a bit hacky, but I have no other ideas for now).

[oojacoboo]

Does it make more sense to handle the generic types in this condition in the TypeHandler?

if ($innerType instanceof Array_ || $innerType instanceof Iterable_ || $innerType instanceof Mixed_) {~

... instead of the else? It seems that if condition is attempting to handle iterable types. Are there types that need generic style typing that aren't an iterable?

[andrew-demb]

Does it make more sense to handle the generic types in this condition in the TypeHandler?

if ($innerType instanceof Array_ || $innerType instanceof Iterable_ || $innerType instanceof Mixed_) {~

... instead of the else? It seems that if condition is attempting to handle iterable types. Are there types that need generic style typing that aren't an iterable?

Seems like yes, it will be a more appropriate place.
I simply moved the check to the mentioned condition.

It is a bit clumsy (since inside condition logic will check dock block type for throwing new exception), but another way may require a check for implementation of \Iterator | \IteratorAggregate (via reflection).

[oojacoboo]

@andrew-demb I'm not sure why I can't reply on the thread with your question. But, as for your question, I'm not sure I understand this error phpDocumentor\Reflection\Type\Object_ is an instance of phpDocumentor\Reflection\Type, which the createForMissingPhpDoc method argument, $type, is typed.

[andrew-demb]

@oojacoboo phpdoc above method narrow down possible type for input parameter

graphqlite/src/Mappers/CannotMapTypeException.php

Line 135 in 53d0018

* @param Array_|Iterable_|Mixed_ $type

And method contain specific handing for every type (to generate proper exception message)

[oojacoboo]

@andrew-demb I see no reason not to expand the phpdoc annotation to support this case.

[andrew-demb]

@andrew-demb I see no reason not to expand the phpdoc annotation to support this case.

The problem was with:

• what exception message should be shown for the developer in that case
• this exception should never happen, since the condition contains a check for not-null doc block type (and specific dock block implementation - Collection)

Since it is not necessary, I didn't change the exception message and just allows to bypass the Object_ instance.

[oojacoboo]

@andrew-demb I see - thanks. In a case like this, I'd just add support and pretend that it could be reached. Future changes could cause that to be the case. Additionally, this method may be reused in other locations. It's best to write a function to handle the logic of its inputs, irregardless of current call stack flows.

[andrew-demb]

Thanks for suggestions @oojacoboo

What can I do to proceed the PR?

[oojacoboo]

What happens with the following generic annotations?

use Acme\Foo;

@return iterable<int, Foo>
@return \Iterator<string, int>
@return \IteratorAggregate<int, bool>
@return \Traversable<string, Foo>

[andrew-demb]

They will be all converted to ListOfType with proper value type.
Unfortunately information about string keys won't be preserved (I didn't find same functionality for IteratorTypeMapper too).

I will add unit tests for these cases to track behavior.

[oojacoboo]

This looks great @andrew-demb. Thanks again for the PR!