overblog / GraphQLBundle

This bundle provides tools to build a complete GraphQL API server in your Symfony App.
MIT License
781 stars 223 forks source link

Be compatible with Symfony Flex #192

Closed renatomefi closed 6 years ago

renatomefi commented 6 years ago
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Version/Branch 0.9.*

Hello,

During the past few days I've been doing some work to make this bundle compatible with symfony flex, the related PRs are:

186 #187 #191

After that I was able to install the bundle easily on it, now I have a PR on symfony flex, please check it: https://github.com/symfony/recipes/pull/183 Edited: Check https://github.com/symfony/recipes-contrib/pull/85 for progress

Please check if it's accordingly to the bundle, I've added configuration for the routes and now it should work out of the box.

Also, releasing a new tag will help the process, since the recipes must determine the minimal version for the package

mcg-web commented 6 years ago

Hi, Nice job @renatomefi ! LGTM, thanks a lot :+1:

renatomefi commented 6 years ago

@mcg-web thanks!

They suggested to open on recipes-contrib, there PR is opened https://github.com/symfony/recipes-contrib/pull/85

Also how to test is described there, looks really plug and play, sadly only on sf 3.3 for now

mcg-web commented 6 years ago

That's a good start... Thanks for your contributions.

danielgrabowski commented 6 years ago

I have another problem with running the bundle with flex. It seems like it doesn't read types configurations from yaml files placed in "src/Resources/config/graphql/". When I open the GraphiQL I get '"Unknown type with alias "Query" (verified service tag)"'. "Query" is a type defined in schema.

renatomefi commented 6 years ago

@danielgrabowski thanks for checking it! Could you paste here all your configuration or how can I reproduce it?

danielgrabowski commented 6 years ago

I tried to make it as simple as possible from scratch. From clean installation I add routes for graphql endpoint and graphiql and fix the graphiql template name (#191). And then (I'm not sure if placement of files is correct since I'm new in Symfony 3 and Flex):

#config/packages/graphql.yml
overblog_graphql:
    definitions:
        schema:
            query: Query
            mutation: ~
#src/Resources/config/graphql/Query.types.yml
Query:
    type: object
    config:
        description: A test query.
        fields:
            hello:
                type: String
                resolve: "world"
renatomefi commented 6 years ago

@danielgrabowski thanks I'll try to check on it!

@mcg-web do you have any clue? I couldn't spot any obvious reason

mcg-web commented 6 years ago

The directory src/Resources/config/graphql/ is not use to auto discover types definition (src/*Bundle/Resources/config/graphql or app/config/graphql should be use)

danielgrabowski commented 6 years ago

The point is that if I'm not mistaken in new directories structure there are no 'src/*Bundle/' or 'app/'.

danielgrabowski commented 6 years ago

Even when I created 'app/config/graphql/' directory it still didn't work.

mcg-web commented 6 years ago

The extension must be yml not yaml, the bundle is not symfony 4 ready yet... I'll try installing the bundle using flex to have better feedback.

danielgrabowski commented 6 years ago

The extension i gave is yml and actually I'm using Symfony 3.3 but installed from symfony/skeleton package with symfony-flex.

renatomefi commented 6 years ago

Indeed I tried the same with sf 3.3 and putting it on app/config/graphql. I'll try to take a better look at it later!

mcg-web commented 6 years ago

@renatomefi what the version of the bundle?

renatomefi commented 6 years ago

@mcg-web

Then try to put the Query.types.yml inside app/config/graphql. This way I was able to reproduce the error. Sadly I didn't have the time to debug yet.

Are you going to look into it?

mcg-web commented 6 years ago

yes i'm going to work on this today

mcg-web commented 6 years ago

@renatomefi when installing i don't have any routes normal?

renatomefi commented 6 years ago

@mcg-web no, the flex recipe copies the routes to: config/routes/graphql.yaml config/routes/dev/graphql_graphiql.yaml

if you execute the same commands as I pasted before that's the expected output. Did all the commands executed nicely?

You have to press y when composer req says it`s a contrib recipe

mcg-web commented 6 years ago

yes nicely, i retry from the start and no routes :(

renatomefi commented 6 years ago

@mcg-web you're right, seems to be a problem with the symfony flex recipes API: https://symfony.sh/r/github.com/symfony/recipes-contrib/85

mcg-web commented 6 years ago

ok I'll retry later

renatomefi commented 6 years ago

@mcg-web if you want you can just copy the routes manually and continue doing the schema do see the issue

mcg-web commented 6 years ago

@renatomefi @danielgrabowski here the path to use to configure project:

sf 3: kernel.root_dir = ./app sf 4 and sf 3.3 using skeleton = ./src

so using ./src/config/graphql fix the issue

mcg-web commented 6 years ago

@renatomefi adding the hello word example in dev (in the recipe) is not so bad, what do you think about that?

renatomefi commented 6 years ago

@mcg-web maybe we can add app/config/graphql to the look up then?

Lemme do a research on the other recipes about it, would be really cool if we could ask during the install:

Would you like to install the hello world? [y/n]
mcg-web commented 6 years ago

app doesn't exist anymore in the new structure maybe we should add it in config... Should first take the time to see how this is treat in other projects.

danielgrabowski commented 6 years ago

@mcg-web from what you wrote for sf 3.3 and 4 {{kernel.root_dir}}/config/graphql would be src/config/graphql. Take a look at this: http://fabien.potencier.org/symfony4-directory-structure.html Fabien wrote there something like this:

Moving templates allows to reserve src/ for PHP classes only, no more resources.

I guess considering this yml files should be placed in config/....

And another thing is that in new sf yaml files have .yaml extension, not .yml.

mcg-web commented 6 years ago

@danielgrabowski you can also use this if you want to be sf 4 ready:

# config/packages/graphql.yml

overblog_graphql:
    definitions:
        schema:
            query: Query
            mutation: ~

overblog_graphql_types:
    Query:
        type: object
        config:
            description: A test query.
            fields:
                hello:
                    type: String
                    resolve: "world"

need some documentation on this...

renatomefi commented 6 years ago

@mcg-web this is the same for SF 3.3 on flex, since it uses the same structure. So for now there is no way to split the type files around?

mcg-web commented 6 years ago

SF 3.3 can be used without flex (flex is compatible with php >= 7.1). But to answer to the question yaml file can be split using include instruction. Even if I think using extension overblog_graphql_types is more respectful of Symfony good practice. I'm open to all subjection, not close to any option.

danielgrabowski commented 6 years ago

Keeping all types in one file will be very unconvenient to maintain with many types, queries and mutations.

mcg-web commented 6 years ago

you can create as many file as needed in config/packages/ or any way else, only requirement is to use extension overblog_graphql_types...

mcg-web commented 6 years ago

The bundle also give possibility to define custom dir the config files must be name and formatted the old way.

danielgrabowski commented 6 years ago

I think I tried to define custom dir and it didn't work with flex. I'll check again and let you know.

renatomefi commented 6 years ago

@danielgrabowski if you find a way let me know, then I can put it in the recipe to make configuration easier

danielgrabowski commented 6 years ago

OK, it seems we have a "go" :) I've placed the type definition files in config/graphql/ and defined custom dir as @mcg-web poited

#config/packages/graphql.yml
overblog_graphql:
    definitions:
        schema:
            query: Query
            mutation: ~
        mappings:
            types:
                -
                    type: yaml # or xml
                    dir: "%kernel.project_dir%/config/graphql"

and it works! A made a clean install using bundle version 0.9.0-BETA4. All I had to do was creating the above graphql.yml file and the type definition. The rest worked out of the box.

renatomefi commented 6 years ago

@danielgrabowski works like a charm indeed!

@mcg-web should we do something to make it auto-discovery inside the bundles? Even though I see there is code for that already mappingFromBundles. I'll see if it still works

mcg-web commented 6 years ago

Symfony 3.3 (with flex) and 4 is bundleless(no AppBundle) so autodiscover in bundle is maybe not needed in this case. I don't really have an answer right now. Autodiscover is a good thing but maybe having hand to disabled/enabled it is also a need...

danielgrabowski commented 6 years ago

@mcg-web is there any easy way to get types from existing Doctine configuration not to repeat describing all the fields?

mcg-web commented 6 years ago

@danielgrabowski not out of the box. GraphQL complementary tools could maybe helps you...

renatomefi commented 6 years ago

@mcg-web for the bundleless we could add:

$typesMappings[] = ['dir' => $container->getParameter('kernel.root_dir').'/../config/packages/overblog-graphql', 'type' => null];

Or even:

$typesMappings[] = ['dir' => $container->getParameter('kernel.project_dir').'/config/packages/overblog-graphql', 'type' => null];

What I'm unsure is whether symfony flex will accept the custom directory, wdyt?

mcg-web commented 6 years ago

I think the best is to use flex config to ease use configuration like :

# config/packages/graphql.yml
overblog_graphql:
    definitions:
        mappings:
            # This part is coming soon
            auto_discover: false
#           auto_discover:
#               bundles: false
#               root_dir: false
            types:
                -
                    type: yaml # this is required rignt now but will accept null in future
                    dir: "%kernel.project_dir%/config/graphql"
renatomefi commented 6 years ago

@mcg-web sure, that's a good idea and more explicit to give user's power! shouldn't it be dir: "%kernel.project_dir%/config/packages/graphql" tho?

Edit: Maybe config/graphql does it, because routes for example is config/routes, I couldn't find docs about it

mcg-web commented 6 years ago

packages is maybe definitely the right place...

mcg-web commented 6 years ago

Wait a minute but the problem the files of this dir will be autoload if the user has a env name graphql so after it should not be place here but out of packages

renatomefi commented 6 years ago

Good point, I'll put it outside and let's see what the recipe review will tell us!

renatomefi commented 6 years ago

@mcg-web I tried the configuration file but now I'm getting: At least one schema should be declare.

danielgrabowski commented 6 years ago

Maybe you should add default

        schema:
            query: Query

in config/packages/graphql.yml and default Query.types.yml. Would it work (not display any error) if Query type don't have any fields?

mcg-web commented 6 years ago

auto_discover settings is not committed yes @renatomefi. Yes we should add a default query i think too... object must have at least one field.

renatomefi commented 6 years ago

@mcg-web even when using this:

overblog_graphql:
    definitions:
        schema:
            query: Query
            mutation: ~
        mappings:
            types:
                -
                    type: yaml # [ yaml, xml ] this is required right now but will accept null in future
                    dir: "%kernel.project_dir%/config/graphql"

it's not working for me

You mean it's not on beta4?