Fixed the custom query not being handled by interface's relations

[EdwinDayot]

Fixes the #421

An Interface requires a public function types returning an array of GraphQL::type() if it contains a relation with a custom query

Still, I'm not a huge fan of this method, so every new idea is appreciated!

[EdwinDayot]

Found out that interfaces public function types were useless

[mfn]

Phu, the changes seem quite complex… I mean, on one side it looks like you just moved stuff around into a dedicated method, but hard to say.

Btw, this one test is failing; what is your test expectation here?

I'm a bit unsure if this change / added complexity is worth it, in its current form (no better suggestion though yet).

[EdwinDayot]

The changes are pretty simple actually. Instead of executing all the code before any database request, in the case of an interface's relation, it is executed on the relation's query, as we don't know what model will be requested. Maybe this solution is better than nothing, and if anyone complains about the complexity, or anything, maybe someone will PR something else?

Anyway, if anyone has something better in mind...

[mfn]

Ok, thx for the info.

I will take a closer look for the review, but that might take a while. If you want to add more details to the source changes (i.e. via github inline comments) to guide the reviewers, that would be much appreciated.

[EdwinDayot]

Added some comments

[mfn]

Thanks for your effort and your patience so far, really appreciated.

I took a longer look today and, whilst not easy, the approach seems 👍 to me!

But my feeling is that it's lacking tests. Only one existing test adapt (which I remember was added in advance for exactly this moment) isn't much.
\Rebing\GraphQL\Support\SelectFields::handleInterfaceFields also seems to have some uncovered parts in the area of if (isset($newParentType->config['types'])) {.

My suggestion would be these steps in order:

• please add much tests as you can think of / possible
• invite others to also give feedback; currently thinking about @crissi as being also quite active lately
• final review and 🤞 merge

How does that sound for your?

PS: without getting ahead, handleInterfaceFields seems like it could use more specific PHP types on the function arguments?

Thanks!

[mfn]

@EdwinDayot I see so often that you've to make micro commits for the StyleCI violations: it should be rather easy => just click on the "Details" and download / apply the patch.

Good news: we want to switch to something else which allows devs to simply run php cs fixer locally, but it's not yet done => #502

[EdwinDayot]

Could not find the issue with Laravel 5.5 for the moment, if anyone has an idea...

EDIT:
A bit of context:

I'm querying

{
  userQuery {
    id
    likes{
      likable{
        id
        title
        likes{
          id
        }
      }
    }
  }
}

the likable field is an interface implemented by Post and Comment types. As you can see, I just want to select the id and title of these, but for some reason - on Laravel 5.5 - it selects * instead

[EdwinDayot]

@EdwinDayot I see so often that you've to make micro commits for the StyleCI violations: it should be rather easy => just click on the "Details" and download / apply the patch.

Good news: we want to switch to something else which allows devs to simply run php cs fixer locally, but it's not yet done => #502

Yeah, that's what I am doing. I wasn't aware of this feature at the beginning, but now, I'm using it :)

[EdwinDayot]

Help wanted

[mfn]

Ops, the StyleCI integration is still active. On it!

[mfn]

Help wanted

I believe there's single "fix" for this problem: Laravel 5.5 simply generates a different query and I've adapted to test to consider a different assertion 🤷‍♀️

Tests are all green now (ignore StyleCI…)

Is this PR ready for review?

I just noticed a lot of whitespace changes on phpdoc in SelectFields and many other kinds of formatting changes => can you please undo this and make "as clean PR as possible"?

[EdwinDayot]

The PHPDoc has been fixed

[mfn]

LGTM 👍

Had a long way coming ;)

I took the liberty and:

• squashed the 155 (!) commits into one…
  I didn't see much value in this kind of history; mostly it was style fixes
• added changelog/readme update
  Would be great if you could check them

I've leave it up for feedback of others a few days more, otherwise I plan to merge it somewhen next week.

[mfn]

Is this change actually related somehow or just a cleanup?

[EdwinDayot]

Just some cleanup :)

[EdwinDayot]

Readme sounds good to me

[mfn]

Preparing the merge now

[mfn]

Had to restart the travis jobs, thus it took longer. They were all green the first time, but the feedback loop to github didn't seem to work 🤷‍♀️ Retriggering the jobs on travis fixed it 👍

Thanks for the PR :)

[EdwinDayot]

Nice :)