Skip to content

Conversation

@cydrickn
Copy link
Contributor

@cydrickn cydrickn commented Oct 29, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@wing328
Copy link
Contributor

wing328 commented Nov 2, 2017

@cydrickn can you elaborate a bit more on the change? Is that a bug fix or enhancement?

cc @naelrashdeen @ksm2 @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

@wing328 wing328 added this to the v2.3.0 milestone Nov 2, 2017
@wing328 wing328 changed the title Update Controller.mustache [PHP][Symfony] Update Controller.mustache Nov 2, 2017
@cydrickn
Copy link
Contributor Author

cydrickn commented Nov 2, 2017

Hi @wing328,

The changes is for php symfony to not response to user the class name and not to response the stack trace of errors for prod environment.
The changes will only response the message of the error if the environment is in production.

Thanks

@wing328
Copy link
Contributor

wing328 commented Nov 3, 2017

@cydrickn thanks for the explanation. If I might, I would suggest adding an "else" clause:

        if ($this->container->get('kernel')->getEnvironment() === 'prod') {
            return [
                'message'  => $exception->getMessage(),
            ];
        } else {
            return [
                'message'  => $exception->getMessage(),
                'type'     => get_class($exception),
                'previous' => $this->exceptionToArray($exception->getPrevious()),
           ];
        }

and also including a 1-liner explaining the difference.

return null;
}

if ($this->container->get('kernel')->getEnvironment() === 'prod') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not test against this, check for debug mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I also prefer the debug flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense seeing this is debug information. Production environment can also be run in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i will change this in using debug

Copy link
Contributor Author

@cydrickn cydrickn Nov 7, 2017

Choose a reason for hiding this comment

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

Done changes in commit
60a2c7a

@wing328 wing328 merged commit fbc2879 into swagger-api:master Nov 9, 2017
@wing328
Copy link
Contributor

wing328 commented Nov 9, 2017

@cydrickn thanks for the PR, which has been merged into master.

I've updated the Petstore sample for PHP Symfony and removed spaces in the empty line 138 via 8b25155

@wing328 wing328 changed the title [PHP][Symfony] Update Controller.mustache [PHP][Symfony] add debug flag to Controller.mustache Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants