Skip to content

Conversation

renatomefi
Copy link
Contributor

@renatomefi renatomefi commented Oct 13, 2017

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Documented? no
License MIT
  • I just want to do some code enhancements and this is a step to symfony 4.0 as well
  • I need some help on how to fix the tests, what's the best way to do it?
  • Obs: I won't do it for GraphiQLController since my plan is to split it

@renatomefi
Copy link
Contributor Author

@mcg-web could you help me with the tests?

@mcg-web
Copy link
Contributor

mcg-web commented Oct 14, 2017

@renatomefi It seems like the service i not properly loaded, I'll give look to this Monday morning, this weekend I'm some way busy 💃

Copy link
Contributor

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

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

Here the changes to make this work

path: /
defaults:
_controller: OverblogGraphQLBundle:Graph:endpoint
_controller: overblog_graphql.controller.graphql:endpoint
Copy link
Contributor

@mcg-web mcg-web Oct 16, 2017

Choose a reason for hiding this comment

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

Add Action suffix the method name is not complete

path: /batch
defaults:
_controller: OverblogGraphQLBundle:Graph:batchEndpoint
_controller: overblog_graphql.controller.graphql:batchEndpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

path: /graphql/{schemaName}
defaults:
_controller: OverblogGraphQLBundle:Graph:endpoint
_controller: overblog_graphql.controller.graphql:endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

path: /graphql/{schemaName}/batch
defaults:
_controller: OverblogGraphQLBundle:Graph:batchEndpoint
_controller: overblog_graphql.controller.graphql:batchEndpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

same here


overblog_graphql.controller.graphql:
class: Overblog\GraphQLBundle\Controller\GraphController
public: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Controller services must be public

use Symfony\Component\HttpFoundation\Response;

class GraphController extends Controller
final class GraphController
Copy link
Contributor

@mcg-web mcg-web Oct 16, 2017

Choose a reason for hiding this comment

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

I'm not for controller as final but this is not part of the issue :)

@mcg-web mcg-web closed this Oct 16, 2017
@mcg-web mcg-web reopened this Oct 16, 2017
@mcg-web
Copy link
Contributor

mcg-web commented Oct 16, 2017

push wrong button 🥇

@renatomefi renatomefi force-pushed the refactor/graphcontroller branch from d90477c to 1bfcad7 Compare October 16, 2017 08:36
@renatomefi renatomefi force-pushed the refactor/graphcontroller branch from 1bfcad7 to 70b9d8a Compare October 16, 2017 08:43
@renatomefi
Copy link
Contributor Author

@mcg-web thanks a lot, your tips were directly to the point! It's working now! :)

@mcg-web
Copy link
Contributor

mcg-web commented Oct 16, 2017

Nice, don't forget to remove wip label if you want it to be merge :)

@mcg-web mcg-web added this to the v0.10 milestone Oct 16, 2017
@renatomefi
Copy link
Contributor Author

@mcg-web sure, I think that was it :)

@mcg-web mcg-web merged commit 5e94b66 into overblog:master Oct 16, 2017
@mcg-web
Copy link
Contributor

mcg-web commented Oct 16, 2017

@renatomefi thank you for 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