[LUMEN] Fix multiple schemas on query/mutation

[Vespira]

Summary

$schema arg received in query function is always null and if we have only one element in $routeParameters, $schema remain empty, then the query always hit default schema.

Type of change

switching condition to fill $schema, from if (count($routeParameters) > 1) to if (count($routeParameters) > 0)

• Bug fix (non-breaking change which fixes an issue)

Checklist

• Existing tests have been adapted and/or new tests have been added
• Add a CHANGELOG.md entry
• Update the README.md
• Code style has been fixed via composer fix-style

Links

• Fixed multiple schema on lumen not find query or mutation #568

[Vespira]

it appears to fail at dependency installation with composer, in this travis Job :

PHP: 7.3
LARAVEL='^6.0' INSTALL_LARAVEL=1 INSTALL_LUMEN=1

But the fact is, I didn't change anything in composer.json or composer-lock file ... is it something that happens frequently ?

[mfn]

is it something that happens frequently ?

Hmm, not frequently but it can…
… still busy and some other PRs have more priority so pleas bear with me here; thx.

[mfn]

But the fact is, I didn't change anything in composer.json or composer-lock file ... is it something that happens frequently ?

The culprit was that the integration tests use laravel/laravel (no version constraint) and Laravel 7 was just released and the test tried to make the library with only supporting Laravel 6 to work => this has just been fixed in master via #597

[mfn]

I don't think the fix is correct, see my feedback…

[mfn]

I don't think the fix is correct here and should rather be within \Rebing\GraphQL\GraphQLController::getRouteParameters, can you investigate?

[Vespira]

I don't think the fix is correct here and should rather be within \Rebing\GraphQL\GraphQLController::getRouteParameters, can you investigate?

Thanks mfn for having a look at my PR :) I don't see what can be fixed in the getRouteParameters as it already handle the Lumen case, and return the only one element in the route param available ..

[mfn]

Yes, so my impression is "if it's broken with Lumen, it need to be fixed in the Lumen code path".

This change here affects all environments == also Laravel, so I'm not confident this is correct (as no one reported problems with Laravel).

Could I explain my position / concerns?

thanks

[Vespira]

Ok I get your point.

The situation here is kind of tricky because with Lumen and only one element in the route URL, the schema is never assigned, and the default schema will always be picked.

Could I write something like this instead :

$routeParameters = $this->getRouteParameters($request);
if (count($routeParameters) > Helpers::isLumen() ? 0 : 1) {
        $schema = implode('/', $routeParameters);
}

This fix wouldn't be necessary if we could pass a schema name directly into function arg :
public function query(Request $request, string $schema = null): JsonResponse {}

[mfn]

if (count($routeParameters) > Helpers::isLumen() ? 0 : 1) {

But you can to the same fix within getRouteParameters in a lumen specific code path, no?

This fix wouldn't be necessary if we could pass a schema name directly into function arg :

That would probably require adapting the routes.php => I'm open to that, but would see some concrete code.

Also we would definitely need a test for this.

[Vespira]

Sorry I'm on two parallel tasks, and I'm fairly new to PHP so I may be annoying. But my feeling here is that the count($routeParameters) > 1 is not right, why would we want to take default schema, if there is one path element anyway ? it still seems valid.

EDIT : I took the time to test a Laravel boilerplate project with graphQL, and saw that we receive the exact same thing from the getRouteParameters method. for instance, if I access a ping query on "test" schema, I will in both case (Laravel and Lumen) receive an array of one element : array("test').

So I decided to fix the root problem and send schema name in query. After a bit of code exploring I didnt found yet the place it's specified 😫

I'll focus on this today, and I'll update the PR to reflect that and update tests

[Vespira]

Ok I took some time again to look at the code, and I finally found something amazing 😲

Into graphql.php config file, if you replace the academic {graphql_schema?} with {schema} in Lumen, it works so hard that I can't even think about how many hours I lost trying to wrap my head around it. I spotted the good place to log into RoutesRequests.php file from Lumen Framework, and I saw that the pattern given as a config key 'routes' in graphql.php was used to insert a into the parameters the schema with this form (e.g private schema ) :
["private": "private"]

Now if we change the config key to {schema} it will then automatically add into parameters :
["schema": "private"]

Hence, the GraphQLController gets hit with correct schema value.

🗡 🗡 I could easily do Sepukku, if we were 600 years back in the past

Jokes aside, I will close this non-sense PR as it works with Lumen, BUT I will open another PR to update documentation about that.