-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[3.0][Console] Added type hint #13756
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
Conversation
------- | ||
8000 |
||
* Parameters of `renderException()` method of the | ||
`Symfony\Component\Console\Application` are type hint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type hinted or have type hints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cordoval thanks!
f5877e2
to
459fa5e
Compare
Isn't this considered a BC break? |
@wouterj If you override For me it's a BC break. |
The BC promise says:
As this is an API method, it can't be done in the 2.x releases. That means this PR should be merged into Symfony 3.0 (the master branch). You don't have to open a PR, as the mergers have tools that can switch the branches quite easily when merging. Btw, I'm +1 for adding the type hints :) |
+1 to merge this in 3.0 too. Accordingly, the content of the |
@fabpot It's a BC break or not? Ready for 2.7 or 3.0? |
It's a BC break yes: any class specializing renderException by inheritance will need an update |
@@ -669,7 +669,7 @@ public function asXml($namespace = null, $asDom = false) | |||
* @param \Exception $e An exception instance | |||
* @param OutputInterface $output An OutputInterface instance | |||
*/ | |||
public function renderException($e, $output) | |||
public function renderException(\Exception $e, OutputInterface $output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BC break, maybe this should be done for the master (3.0) instead?
@francisbesset This is indeed a BC break and can only be merged in 3.0... but is it worth it? |
I'm in favor of merging it in 3.0 |
I don't see why not to merge it into 3.0. |
459fa5e
to
542e7ff
Compare
I updated the title of the PR to specify 3.0 but I can't update the base branch for the merge. |
Thank you @francisbesset. |
This PR was submitted for the 2.7 branch but it was merged into the 3.0-dev branch instead (closes #13756). Discussion ---------- [3.0][Console] Added type hint The method `Symfony\Component\Console\Application::renderException()` have 2 parameters not type hint but the PHPDoc reveal types. | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | - Commits ------- e98c537 [Console] Added type hint
ping @francisbesset see #15657 |
The method
Symfony\Component\Console\Application::renderException()
have 2 parameters not type hint but the PHPDoc reveal types.