8000 [3.0][Console] Added type hint by francisbesset · Pull Request #13756 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

francisbesset
Copy link
Contributor

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 -

@francisbesset francisbesset changed the title [Console] Added type hint [2.7][Console] Added type hint Feb 21, 2015
-------

8000
* Parameters of `renderException()` method of the
`Symfony\Component\Console\Application` are type hint.
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cordoval thanks!

@wouterj
Copy link
Member
wouterj commented Feb 22, 2015

Isn't this considered a BC break?

@francisbesset
Copy link
Contributor Author

@wouterj If you override Application::renderException() you should specify $e and $output without type hint.
This PR fix the type hint and you will update your renderException method to add the type hint.

For me it's a BC break.

@wouterj
Copy link
Member
wouterj commented Feb 22, 2015

The BC promise says:

Add type hint to an argument: NO (in case of an API method)

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 :)

@dunglas
Copy link
Member
dunglas commented Feb 22, 2015

+1 to merge this in 3.0 too. Accordingly, the content of the UPGRADE file must be moved in 3.0.

@francisbesset
Copy link
Contributor Author

@fabpot It's a BC break or not? Ready for 2.7 or 3.0?

@nicolas-grekas
Copy link
Member

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)
Copy link
Contributor

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?

@fabpot
Copy link
Member
fabpot commented Feb 23, 2015

@francisbesset This is indeed a BC break and can only be merged in 3.0... but is it worth it?

@Tobion
Copy link
Contributor
8000
Tobion commented Feb 23, 2015

I'm in favor of merging it in 3.0

@linaori
Copy link
Contributor
linaori commented Feb 23, 2015

I don't see why not to merge it into 3.0.

@francisbesset francisbesset changed the title [2.7][Console] Added type hint [3.0][Console] Added type hint Feb 23, 2015
@francisbesset
Copy link
Contributor Author

I updated the title of the PR to specify 3.0 but I can't update the base branch for the merge.

@fabpot
Copy link
Member
fabpot commented Feb 24, 2015
8000

Thank you @francisbesset.

@fabpot fabpot closed this Feb 24, 2015
fabpot added a commit that referenced this pull request Feb 24, 2015
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
nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Aug 30, 2015
…besset)"

This reverts commit ad1888b, reversing
changes made to fffcc24.
@nicolas-grekas
Copy link
Member

ping @francisbesset see #15657

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.

8 participants
0