Skip to content

[HttpKernel] change $previous argument for HttpException to \Throwable #30729

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 30, 2019
Merged

[HttpKernel] change $previous argument for HttpException to \Throwable #30729

merged 1 commit into from
Mar 30, 2019

Conversation

sGy1980de
Copy link
Contributor

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30728
License MIT

This will fix #30728 with the suggested solution to change the signature of HttpException and all its descendants from \Exception to \Throwable.

@nicolas-grekas
Copy link
Member

Thanks, LGTM. Should target master as it's a new feature.

@sGy1980de
Copy link
Contributor Author

@nicolas-grekas
Could you please explain a little, why you consider this a new feature? To me it is a bug or at least unintended behaviour. Furthermore this won't change any existing application out there, so i really don't get the point here.

@nicolas-grekas
Copy link
Member

This never worked as you said, so it wasn't supported. Adding support for something that wasn't possible before is almost the definition of a new feature to me.

@sGy1980de
Copy link
Contributor Author

So what's next?
@javiereguiluz marked the corresponding issue as bug. Shall i raise it with target master again or let it like that?

@javiereguiluz
Copy link
Member

@sGy1980de I marked it as a bug because the issue description looked as a bug. But I could be wrong, so please don't think that it's definitive.

@sGy1980de
Copy link
Contributor Author

Hey guys - what about the bug vs. feature discussion on this? Is it discussed internally or are all following @nicolas-grekas opinion and i should raise this with target master again?

@fabpot
Copy link
Member

fabpot commented Mar 29, 2019

I tend to agree with @nicolas-grekas here.

@fabpot fabpot changed the base branch from 4.2 to master March 30, 2019 07:30
@fabpot
Copy link
Member

fabpot commented Mar 30, 2019

Thank you @sGy1980de.

@fabpot fabpot merged commit 15cb475 into symfony:master Mar 30, 2019
fabpot added a commit that referenced this pull request Mar 30, 2019
@fabpot
feature #30729 [HttpKernel] change $previous argument for HttpExcepti…
e9e2f21 
…on to \Throwable (sGy1980de)

This PR was submitted for the 4.2 branch but it was squashed and merged into the 4.3-dev branch instead (closes #30729).

Discussion
----------

[HttpKernel] change $previous argument for HttpException to \Throwable

| Q             | A
| ------------- | ---
| Branch?       |  4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30728
| License       | MIT

This will fix #30728 with the suggested solution to change the signature of `HttpException` and all its descendants from `\Exception` to `\Throwable`.

Commits
-------

15cb475 [HttpKernel] change $previous argument for HttpException to \Throwable
@fabpot fabpot mentioned this pull request Mar 30, 2019
Merged
fabpot added a commit that referenced this pull request Mar 30, 2019
@fabpot
feature #30780 Fix some exception previous type hints (fabpot)
760bbd5 
This PR was merged into the 4.3-dev branch.

Discussion
----------

Fix some exception previous type hints

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

follow-up for #30729

Commits
-------

f92efeb fixed some exception previous type hints
@fabpot fabpot mentioned this pull request May 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[HttpKernel] HttpException does not allow \Error as $previous argument
6 participants
@sGy1980de @nicolas-grekas @javiereguiluz @fabpot @stof @carsonbot