Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to use array in controller param in config #906

Conversation

viktorruskai
Copy link
Contributor

@viktorruskai viktorruskai commented Mar 4, 2022

Summary

  • This PR contains a small feature where you can use an array in the config file. It will help your IDE navigate to the method you are using.
return [
    'route' => [
        // The prefix for routes; do NOT use a leading slash!
        'prefix' => 'graphql',

        //  Normal format: 
        // 'controller' => GraphQLController::class . '@query',

        // Array format:
        'controller' => [GraphQLController::class, 'query'],
        ...

        'graphiql' => [
               'controller' => [GraphQLController::class, 'graphiql'],
               ...
        ],
        ...
  • In your IDE it should look like this:

Screenshot 2022-03-04 at 15 46 34


Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

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

@viktorruskai
Copy link
Contributor Author

@mfn I updated the Pull request.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Can you please:

  • work in my feedback
  • add an example for array syntax in the config itself, .e. g
            // The controller/method to use in GraphQL request.
            // Also supported array syntax: `[\Rebing\GraphQL\GraphQLController::class, 'query']`
            'controller' => \Rebing\GraphQL\GraphQLController::class . '@query',
    
  • resolve the conflicts with master?

After that, I'm happy to merge the PR!

README.md Outdated
@@ -2655,7 +2655,7 @@ To prevent such scenarios, you can add the `UnusedVariablesMiddleware` to your
The default makes the API available via `/graphql`
- `controller`\
Allows overriding the default controller class, in case you want to extend or
replace the existing one.
replace the existing one. (also supports `array` format)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
replace the existing one. (also supports `array` format)
replace the existing one (also supports `array` format).

@viktorruskai
Copy link
Contributor Author

@mfn I implemented everything you mentioned.

@viktorruskai viktorruskai requested a review from mfn May 5, 2022 09:30
@mfn
Copy link
Collaborator

mfn commented May 5, 2022

I guess we've to run composer phpstan-baseline to update the baseline; not sure what the appropriate code/phpdoc fix would be and I would be fine adding it to the baseline.

(note: I generated it locally but I can't push to your branch)

@viktorruskai
Copy link
Contributor Author

@mfn Yeah, sure ... I updated the baseline. Also, I tried to set proper PHPDoc with PHPStan Array Shape, but unsuccessfully.

@mfn
Copy link
Collaborator

mfn commented May 5, 2022

@viktorruskai thanks!

Unfortunately the baseline needs to be generated with the same PHP version running on GHA, which is 8.1 and not 7.4

I fixed this with #916 , which included your PR 🎉

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants