Fix use of MagicField with phpType on a proxy class without magic __get method

[michael-vostrikov]

Closes #404

[codecov-commenter]

Codecov Report

Merging #405 (2c4ffd3) into master (b5230f7) will decrease coverage by 0.04%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master     #405      +/-   ##
============================================
- Coverage     98.30%   98.25%   -0.05%     
- Complexity     1578     1580       +2     
============================================
  Files           147      147              
  Lines          4241     4246       +5     
============================================
+ Hits           4169     4172       +3     
- Misses           72       74       +2     

Impacted Files	Coverage Δ
src/FieldsBuilder.php	96.37% <71.42%> (-0.56%)	⬇️

---

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 b5230f7...2c4ffd3. Read the comment docs.

[oojacoboo]

This probably needs to be handled some other way. Currently, as it's written, if there isn't a __get magic method, instead of failing, it's simply assigning null. It's better to fail than to assign something that wasn't intended.

Can you please outline your proxy class example here?

[michael-vostrikov]

It is like in test, just a proxy class with MagicField with phpType.
https://github.com/thecodingmachine/graphqlite/pull/405/files#diff-95511f6323fe084d6ab0e77ef3c9c71b20247b1e26c34c0e1d7b0bcf5880d4d0

$refMethod is required in resolvePhpType() and mapReturnType(), so it should be some real method. The right way would be using a __get() method from source class, but it is not passed into getQueryFieldsFromSourceFields(). Anyway it is used as a stub, real type is taken from $phpTypeStr, so I thought a fake method would be good enough.

[oojacoboo]

Okay, so after reviewing a bit more, I think I see what you're doing here. This is an extended type class, right? It's not exactly a "proxy class"? Assuming I'm understanding your intent properly, I don't have an issue with getting a proper fix for this use case. However, the current implementation is quite bad.

Firstly, nested if statements are a big red flag. But more importantly, the if (! $sourceField->shouldFetchFromMagicProperty()) evaluation should be returning false, letting the magic property logic handle the assignment.

The submitted code hacks something into place that happens to work for your use case. We need something that works with the existing logic.

[michael-vostrikov]

This is an extended type class, right?

No, according to documentation, TestTypeWithMagicPropertyType is a proxy class for TestTypeWithMagicProperty, just instead of SourceField (which works with methods) I use MagicField (which works with properties).

TestTypeWithMagicProperty is just a class with __get method, like Eloquent model in Laravel, it should not necessarily be a GraphQL type.

We need something that works with the existing logic.

Existing logic contains a bug, pull request fixes it. Try to comment changes in FieldsBuilder and run the test. You will get PHP exception "ReflectionException: Method __get does not exist", though by documentation this is a valid construction. It works with MagicField(outputType) but does not with MagicField(phpType).

Instead of fake __get we can change resolvePhpType, mapReturnType and other methods so that they can accept null as a $refMethod, but this would be a lot more changes. If this is ok, I'll do it.

[oojacoboo]

@michael-vostrikov thanks for the clarity. Regarding the changes, my comments remain the same. The existing logic may not work in this case, which I understand. However, your changes are a hack, and not a good one. The changes need to be worked into the existing logic in a better way, as I described.

[michael-vostrikov]

Found a way how to get source class and magic __get method in it.

[oojacoboo]

@michael-vostrikov this is much better! Can we find a more appropriate method name for findMagicGetMethodContainer? Maybe something like getRefClassOrProxy? Also, it's best to use the conditional check, if (! $controllerRefClass->hasMethod('__get')) with an early return. This keeps the code cleaner.

[michael-vostrikov]

I did it a little wrong, $refClass should always be a proxy class to create Context for it, because it's where we write class name in phpType, and where we can write use for this class. There may not be this use in a source class. So the method name now is getMagicGetMethodFromSourceClassOrProxy.

[oojacoboo]

This looks great @michael-vostrikov. Thanks for the PR and the follow-up work on it!